ci: cover a build without MRB_UTF8_STRING - #7138
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CI configuration adds a default-gembox build. It runs byte-indexed string and regexp tests without UTF-8 string support. Binary tests remain disabled. ChangesCI coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This adds a CI build for the default gembox to cover byte-indexed strings. It increases CI work and leaves less timeout margin on mingw, but the reported builds pass and no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
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 |
Every build CI runs takes the full-core gembox. full-core carries mruby-encoding, and that gem is what defines `MRB_UTF8_STRING`, so no job in the matrix builds a mruby whose strings index by byte. The tests written as the byte-indexed mirror of the UTF-8 ones therefore never ran. `String#scrub no-op on non-UTF-8 build` skipped itself on every build, and so did the byte-counting halves of mruby-regexp and mruby-string-ext. Both gems name that missing build in their mrbgem.rake: mruby-string-ext says UTF-8 coverage comes from full-core while "the mirror runs on gemboxes without it", and mruby-regexp sets MRB_UTF8_SCAN because "the default gembox carries this gem without" mruby-encoding. Nothing built the default gembox. The cosmopolitan job does take a gembox without mruby-encoding, but its build config enables neither `enable_test` nor `enable_bintest`, so `rake test:run:serial` builds no mrbtest for it and runs no test there. Add a fourth build on the default gembox to the CI build config. It rides the existing matrix instead of taking a job of its own, so it costs no runner and gets checked on Linux, macOS and mingw, under gcc and clang, on ARM64 and on the one runner where `char` is unsigned. It enables tests only, because the binaries the default gembox adds are the ones the bintest on the build above already covers. The build adds 270 compilation units to the 883 the three existing ones come to. Over the last eight master runs of the workflow the slowest job in the matrix, windows mingw-gcc, took between 5m08s and 6m07s and every other job finished inside three minutes, so a third more compile work leaves `timeout-minutes: 10` as it is.
efb8467 to
5fe6a01
Compare
Problem
Every build in CI takes the
full-coregembox: the three inbuild_config/ci/gcc-clang.rband the one inbuild_config/ci/msvc.rb.full-corepulls in mruby-encoding, and that gem is what definesMRB_UTF8_STRING:So CI has never built a mruby whose strings index by byte, and the tests
written as the byte-indexed mirror of the UTF-8 ones have never run.
String#scrub no-op on non-UTF-8 buildguards itself withskip if "あ".length == 1and skipped on every job in the matrix. Thebyte-counting halves of mruby-regexp and mruby-string-ext went the same
way.
Both gems assume that build exists. mruby-string-ext:
and mruby-regexp, on why it sets
MRB_UTF8_SCAN:Nothing in CI builds the default gembox. The Cosmopolitan job comes
closest, taking the stdlib gemboxes with no mruby-encoding, but
build_config/cosmopolitan.rbenables neitherenable_testnorenable_bintest, sorake test:run:serialbuilds no mrbtest for it andruns no test there.
Change
A fourth build in
build_config/ci/gcc-clang.rbon the default gembox.It rides the existing matrix rather than taking a job of its own, the
way the
MRB_REGEXP_UNICODE_CASEcoverage onfull-debugdoes, so itcosts no runner and gets checked on Linux, macOS and mingw, under gcc
and clang, on ARM64 and on the one runner where
charis unsigned. Itenables tests only: the binaries the default gembox adds are the ones
the bintest on the build above already covers.
Verification
rake -m test:run:serial MRUBY_CONFIG=ci/gcc-clangfrom a clean builddirectory, on Linux with gcc:
The new build passes as it stands, so this adds coverage without
uncovering a failure.
Cost
The new build comes to 270 compilation units against the 883 the three
existing ones come to together, so it adds about a third to the compile
work of every job in the GCC-CLANG matrix.
Job durations over the last eight master runs of this workflow, as a
reference point rather than a prediction, since runners vary:
A third more compile work on the worst mingw run seen there comes out
near eight minutes, inside the 10 minute timeout, and that scaling is
pessimistic because a job's checkout and setup do not grow with the
build. Everything outside mingw sits under three minutes either way.
timeout-minutes: 10is left alone, but mingw is where the margin isthinnest, so raising it is a one line change if you would rather have
the room.
Note on the four warnings
The new build reports 4 "no assertion" warnings, all in
mrbgems/mruby-string-ext/test/string.rb, where a test wraps its wholebody in
if UTF8STRINGand so runs empty when strings index by byte:String#inspect of a binary string escapes every byteString#chop! on a binary string removes one byteString#rindex on a binary string counts bytesa needle that spells no character is found nowhereThey are warnings rather than failures, and CI stays green. The rest of
that file writes the same condition as
end if UTF8STRING, whichreports nothing. A separate PR will bring those four into that form.
Summary by CodeRabbit