ci: read the byte-indexed side on full-core minus mruby-encoding - #7155
Merged
Conversation
`MRuby::Gem::List` had `[]` and `<<` but no way to remove, so a build config that wanted a gembox minus one gem had to restate the box. `build_config/i586-pc-msdosdjgpp.rb` does exactly that: twelve lines hand-expanding `default.gembox`, each carrying a comment naming the box the line came from, so that `mruby-socket` can be left out. `conf.gems.reject!` was the natural reach for this and raised NoMethodError; `conf.gems.reject` inherits from Enumerable, returns a new Array and drops it, so the removal reads as if it worked and does nothing. Add `List#delete`, which names the gem, and `List#reject!`, which takes a predicate. Both run while the build config is being read, before any `Specification#setup`, so a removed gem contributes nothing: not its objects, and not the defines its `mrbgem.rake` sets on the build. Measured on `full-core` minus `mruby-encoding`: 57 gems instead of 58, and `MRB_UTF8_STRING` and `HAVE_MRUBY_ENCODING_GEM` both absent. `delete` fails when the name is not in the build rather than returning nil, following `Can't find gembox` and `Invalid gem name` elsewhere in the build system. A misspelled name is a typo, and a build that silently keeps the gem is the failure this method exists to remove. `reject!` keeps Array semantics and returns nil when it matches nothing, since a predicate matching nothing is not a mistake.
`List#delete` runs while the build config is read, and dependencies are
declared later, in `Specification#setup`. So a gem cannot be checked
against them at the moment it is removed, and one that another gem in
the build depends on is loaded again by `setup_dependencies` a phase
later. `conf.gems.delete 'mruby-string-ext'` on `full-core` ends with
the same 58 gems it started with, and says nothing.
Keeping the gem is right, since `mruby-regexp` cannot be built without
it. Staying quiet is not: the build config asked for something and got
the opposite, which is the shape of failure `delete` was added to
remove.
Record what was removed and say so when it comes back, naming the gem
that requires it:
gem 'mruby-string-ext' can't be removed; mruby-regexp depends on it
23 gems in a `full-core` build with tests enabled are reachable this
way, 17 without, the difference being `add_test_dependency`, which is
`add_dependency` under `test_enabled? || bintest_enabled?`.
`mruby-encoding` is in the larger list but is still removable, because
`mruby-regexp` and `mruby-sprintf` guard that dependency on the gem
being present and the guard is read after the removal.
The fourth build in `ci/gcc-clang` exists so that the tests written as
the byte-indexed mirror of the UTF-8 ones run somewhere. It reached that
by taking the default gembox, which is the smaller box that happens not
to carry `mruby-encoding`. What it wants is the gem gone, not a smaller
box, and `conf.gems.delete` says that.
On `full-core` minus `mruby-encoding`: 2220 tests instead of 2066, the
same 26 skips, and 9 more gems under test (`mruby-benchmark`,
`mruby-bin-mrb`, `mruby-cmath`, `mruby-exit`, `mruby-os-memsize`,
`mruby-strftime`, `mruby-string-bitops`, `mruby-task`,
`mruby-test-inline-struct`). The one gem the default gembox had and
`full-core` does not is `mruby-bin-debugger`, which the `bintest` build
in this file already adds by name.
The build is renamed `byte-string` after what it reads, since it is no
longer the default gembox. It answers as a byte-indexed build should:
$ build/byte-string/bin/mruby -e 'p "あ".length, "あ"[0], defined?(Encoding)'
3
"\xe3"
nil
No CI build loads `mrbgems/default.gembox` after this. The five boxes it
nests are still loaded by the Cosmopolitan job, which names them
directly; what loses coverage is the six lines of the wrapper.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Gem list now supports explicit removal with dependency re-addition warnings. Compile documentation describes removal rules. The CI configuration adds a byte-string build based on ChangesGem removal and byte-string build configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR expands byte-indexed CI coverage by using full-core without mruby-encoding, with the reported builds completing with zero failures; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant CompileConfig
participant GemList
participant DependencyResolution
CompileConfig->>GemList: delete a named Gem
GemList-->>CompileConfig: remove the Gem or raise an error
DependencyResolution->>GemList: resolve dependencies
GemList-->>DependencyResolution: provide requiring Gem metadata
DependencyResolution-->>GemList: warn and re-add a removed dependency
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #7154, whose two commits this branch carries. Only the last commit is new here.
The fourth build in
ci/gcc-clangexists so that the tests written as the byte indexed mirror of the UTF-8 ones run somewhere:String#scrubdegrading to a no-op, the byte counting halves ofmruby-regexpandmruby-string-ext. It reached that by taking the default gembox, which is the smaller box that happens not to carrymruby-encoding. What it wants is the gem gone, not a smaller box, and with #7154 it can say so.This is the configuration you built in #7142 when you showed that a byte indexed full-core build is available today. It answers the same way:
What it changes
mruby-encoding154 more tests at the same skip count. Nine gems come under byte indexed test that were not:
mruby-benchmark,mruby-bin-mrb,mruby-cmath,mruby-exit,mruby-os-memsize,mruby-strftime,mruby-string-bitops,mruby-task,mruby-test-inline-struct. The one gem the default gembox had andfull-coredoes not ismruby-bin-debugger, which thebintestbuild in this file already adds by name, so no gem loses a build.The build is renamed
byte-stringafter what it reads, since it is no longer the default gembox.What it costs
No CI build loads
mrbgems/default.gemboxafter this. The five boxes it nests are still loaded, by the Cosmopolitan job, which namesstdlib,stdlib-ext,stdlib-io,mathandmetaprogdirectly; what loses coverage is the six lines of the wrapper.#7138 added this build for the byte indexed tests rather than for the gembox, and before it no CI build read
default.gemboxeither, so this is a return to that rather than a new gap. Still, it is a real one, and if you would rather keep a build on the box I can add it back as a fifth rather than trade it.Tests
MRUBY_CONFIG=ci/gcc-clang rake -m test, all four builds and the bintests, KO 0:No
can't be removedwarning is printed.mruby-regexpandmruby-sprintfdeclaremruby-encodingas a test dependency only when it is present, and that guard is read after the removal.Summary by CodeRabbit