build: build the Prism objects through the rules - #7248
Conversation
mruby-compiler compiled the Prism sources with a `file` task of its own instead of the rules every other object of a gem is built by. A `file` task has the prerequisites it is written with, here the source alone, so nothing read the `.d` a compile of a Prism object writes and nothing compared the flags the object was compiled with against the flags of now. The 21 Prism objects of a build (42 with the `mrbc` build) were the only objects outside both: - A change to a Prism header rebuilt the compiler glue and not Prism. `touch lib/prism/include/prism/defines.h`, then `rake`: 70 objects recompiled, the glue and the regenerated `gem_init.c` / `mrblib.c`, and 0 of the 42 Prism objects. - A change to the compile flags rebuilt every object but Prism. `conf.cc.defines << "PRISM_DEPTH_MAXIMUM=64"` in the config, then `rake`: 165 objects recompiled, 0 of the 42 Prism objects, and the `mruby` it linked still parsed an expression nested 100 deep, since the cap lives in `prism.c` and `prism.o` kept the cap it was built with. The record beside the object said `PRISM_DEPTH_MAXIMUM=256`. Register the objects with `Compiler#define_rules`, as the objects of every other gem are, so that they get the header prerequisites of their `.d` and the flags comparison of every rule-built object. Both scenes above now recompile the 42 Prism objects (112 and 207 objects), the `rake` after them does nothing, and the nested expression is refused with `nesting too deep`. Under `MRB_USE_CXX_ABI` the Prism sources compile with a clone of the gem's `cc`, the C++ compile flag stripped and `MRC_ALLOC_LIBC` added, and the clone must be made once `cc` is complete: `Compiler#clone` is a deep copy, and the mrbgem.rake body runs before the gem's version define, the include paths of the gems it depends on and the presym include path reach `cc`. The `file` task cloned inside its action for that reason. `define_rules` now takes a block that names the compiler when a rule is resolved, and mruby-compiler derives its compiler in that block, once. Without a block the rules compile with the receiver as before. `get_dependencies` becomes protected so the rules one compiler defines can ask the compiler they run. The objects mirror the Prism source tree, `lib/util/pm_buffer.o` for `src/util/pm_buffer.c`, since a rule maps an output to its source by path; the `file` task flattened them into `lib/`.
|
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 (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesCompiler rule selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change makes Prism objects rebuild when relevant headers or compile flags change, with supplied default and C++ ABI checks passing; no actionable merge-blocking risk remains after normal review. Sequence Diagram(s)sequenceDiagram
participant cc
participant Compiler#define_rules
participant Prism compiler
cc->>Compiler#define_rules: Register Prism C sources
Compiler#define_rules->>Prism compiler: Resolve dependencies and execute rules
Prism compiler-->>Compiler#define_rules: Generate Prism object files
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 |
mruby-compiler compiles the Prism sources with a
filetask of its owninstead of the rules every other object of a gem is built by. A
filetaskhas the prerequisites it is written with, here the source alone, so nothing
reads the
.da compile of a Prism object writes and nothing compares theflags the object was compiled with against the flags of now. The 21 Prism
objects of a build (42 with the
mrbcbuild) are the only objects outsideboth, and they stay as they are through a change to a Prism header and
through a change to the compile flags.
The flags case is visible from the outside.
PRISM_DEPTH_MAXIMUMis thenesting cap Prism refuses input at, and mruby-compiler's mrbgem.rake sets it
to 256 unless the config sets it first. Setting it in the config of a build
directory that was built once leaves the cap where it was:
Why
Dir.globover the Prism sources registers each object withfile obj => [src] do prism_cc.run ... end. Rake takes the task as it iswritten, so the object is up to date whenever it is newer than its
.c;the
-MMDoutput beside it is written and never read, andCompiler#get_dependencies, which reads the.dand discards an outputwhose recorded flags differ (#7236), runs from the rules only. Everything
else a gem builds, its own sources and its generated
gem_init.c, goesthrough
Compiler#define_rules.The
filetask was where the compiler for the C++ ABI build was made: aclone of the gem's
ccwith the C++ compile flag stripped andMRC_ALLOC_LIBCadded, cloned inside the action becauseCompiler#cloneis a deep copy and the mrbgem.rake body runs before the gem's version
define, the include paths of the gems it depends on and the presym include
path reach
cc. A rule defined in the body would have had to name itscompiler in the body.
What this does
Compiler#define_rulestakes a block that names the compiler to runwith. The block is called when a rule is resolved, not when the rules
are defined, so a compiler it derives from the receiver sees everything
the build added to the receiver since. Without a block the rules run
with the receiver, as before.
get_dependenciesbecomes protected sothe rules one compiler defines can ask the compiler they run.
define_rulesandderives the C++ ABI compiler in the block, once. The objects mirror the
Prism source tree (
lib/util/pm_buffer.oforsrc/util/pm_buffer.c),since a rule maps an output to its source by path; the
filetaskflattened them into
lib/.The compile lines do not change: every Prism object is byte-identical to
the one
masterbuilds at the same path, in the default and in the C++ABI build, and so is
bin/mrbc;bin/mrubydiffers only in the debugline strings that spell the build directory.
Verified
rake -m -j16,CCACHE_DISABLE=1, gcc; the build directory built once onmaster(0122b45) before each row, and the run after each row doesnothing in the "this PR" column.
touch lib/prism/include/prism/defines.hCC: the compiler glue and the regeneratedgem_init.c/mrblib.c, 0 of the 42 Prism objectsCC: the same 70 and the 42 Prism objectsconf.cc.defines << "PRISM_DEPTH_MAXIMUM=64"in the configCC, 0 Prism;bin/mrubyaccepts the expression nested 100 deep,prism.o.flagssays 256CC, 42 Prism;bin/mrubyrefuses it withnesting too deep,prism.o.flagssays 64conf.cc.defines << "PROBE_FLAG"in the config, C++ ABI build (enable_cxx_abi)CC, 0 PrismCC, 42 Prism; the Prism record has-DMRC_ALLOC_LIBCand the presym include path and no-x c++, the glue record has-x c++rake -m -j16 testfrom an empty build directory,default.rbwithenable_bintestdefaultgembox,enable_cxx_abi)The 207 is 208 objects less
build/host/mrbgems/gem_init.o, whosefiletask in
tasks/mrbgems.rakehas no action and takes the rule's at executetime, after Rake has decided it is up to date; that one is outside this PR.
Environment
Details
The compile line of
lib/prism.oin the default build, unchanged by thisPR (the record beside the object;
%{flags}is theflags:line):gcc -MMD -c -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DPRISM_XALLOCATOR -DPRISM_DEPTH_MAXIMUM=256 -DMRC_TARGET_MRUBY -DPRISM_BUILD_MINIMAL -DMRBGEM_MRUBY_COMPILER_VERSION=0.0.0 -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DMRB_USE_COMPLEX -DMRB_USE_BIGINT -DMRB_USE_DEBUG_HOOK -I"include" -I"mrbgems/mruby-compiler/include" -I"mrbgems/mruby-compiler/lib/prism/include" -I"mrbgems/mruby-compiler/include" -I"build/host/include" -o "build/host/mrbgems/mruby-compiler/lib/prism.o" "mrbgems/mruby-compiler/lib/prism/src/prism.c"The same object in the C++ ABI build, next to the glue that is built as
C++:
No C source changes and the compile lines are the same, so
.textisunaffected in every build.
Summary by CodeRabbit