Order ext/configure-ext.mk after prepare-gems#4
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1662ca071d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Per @chatgpt-codex-connector review on #4: making ext/configure-ext.mk unconditionally depend on prepare-gems meant plain \`make\`/\`main\` builds would chain into update-gems, which fetches bundled-gem archives over the network. That breaks offline builds on a fresh checkout and mutates the source tree in a way unrelated to the requested target. Move the dep to defs/gmake.mk under a MAKECMDGOALS filter for the docs targets (rdoc, rdoc:%, html, html-server, rdoc-coverage), which are the only ones whose recipes need bundled-gem extensions activatable. Plain \`make\` no longer pulls the prepare-gems chain. Verified: - \`make -n html\` shows "Extracting bundle gem ..." lines - \`make -n\` (plain) shows no extraction or download - \`rm -rf .bundle && make html\` still completes (13895 items) https://claude.ai/code/session_01YRoRyZPew2LtN6u6BX8bzx
|
@codex review I feel that the current implementation is workarounds spread across different places for different issues. Can you find a more elegant solution. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce85b0359b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ifneq ($(filter rdoc rdoc\:% html html-server rdoc-coverage,$(MAKECMDGOALS)),) | ||
| ext/configure-ext.mk: $(HAVE_BASERUBY:yes=prepare-gems) |
There was a problem hiding this comment.
When invoking the existing query target as make 'rdoc:Integer#+', MAKECMDGOALS contains rdoc:Integer#+, but this $(filter rdoc\:% ...) entry does not match it in GNU make because : is not special in filter patterns and the backslash is treated literally. As a result the new prepare-gems prerequisite is skipped for the very rdoc:% goal listed here, so a clean tree can still generate ext/configure-ext.mk before bundled gems are extracted and hit the same rdoc/rbs activation failure.
Useful? React with 👍 / 👎.
After b5e6e0a (Use rbs-integrated RDoc), tool/rdoc-srcdir does require 'rdoc/rdoc' which activates rdoc-7.2.0 and its new add_dependency 'rbs', '>= 4.0.0'. RubyGems then needs rbs's gemspec in .bundle/specifications/ and its C extension built. ext/extmk.rb already handles both via each per-gem Makefile (all -> install -> gemspec copies .bundled.rbs-*.gemspec into specifications/, and the TARGET_SO recipe writes gem.build_complete) -- but only if ext/configure-ext.mk has seen .bundle/gems/ populated when its template's Dir.glob(".bundle/gems/**/extconf.rb") runs. On a clean tree, ext/configure-ext.mk regenerates before prepare-gems extracts anything, so its template glob finds nothing, exts.mk lacks rules for bundled-gem extensions, the rbs C extension never compiles, and RubyGems aborts with "Ignoring rbs-4.0.2 because its extensions are not built." Add prepare-gems as a prereq of ext/configure-ext.mk, scoped via MAKECMDGOALS to docs goals only (rdoc, rdoc:%, html, html-server, rdoc-coverage, undocumented). Plain `make` is untouched, so an offline-fresh-checkout build doesn't reach prepare-gems -> update-gems. Also fix tool/update-deps: with the new dep, `make -p all` now includes rules from inside .bundle/gems/<gem>/ext/<ext>/, where make prints relative targets like `bigdecimal.o:` whose curdir is under .bundle/. The existing filter checked the raw target string, missing these; move the check onto `dir + target` so the full path is what's inspected. Bundled gems own their depend files, so skipping them here matches the intent of the original filter. https://claude.ai/code/session_01YRoRyZPew2LtN6u6BX8bzx
st0012
force-pushed
the
claude/investigate-docs-build-lnnIc
branch
from
ce85b03 to
5affd5f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5affd5fab8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # prepare-gems may not have run yet, so make the dep explicit for the | ||
| # docs goals. Other targets are left alone so a plain `make` doesn't | ||
| # reach for prepare-gems -> update-gems (network). | ||
| ifneq ($(filter rdoc rdoc:% html html-server rdoc-coverage undocumented,$(MAKECMDGOALS)),) |
There was a problem hiding this comment.
When documentation is requested through aggregate targets such as make docs or make install with INSTALLDOC=all, MAKECMDGOALS contains only docs/install, even though common.mk routes those targets to $(DOCTARGETS)/rdoc and srcs-doc. Because this guard stays false in that scenario, a parallel clean-tree docs install can still let main generate ext/configure-ext.mk before srcs-doc runs prepare-gems, leaving the rbs extension out of exts.mk and preserving the activation failure this change is meant to fix.
Useful? React with 👍 / 👎.
Per @chatgpt-codex-connector review on #4: docs is also reached via \`make docs\` (which routes to srcs-doc + \$(DOCTARGETS)) and via \`make install INSTALLDOC=all\` (do-install-all has docs in its prereqs). In those cases MAKECMDGOALS only contains \`docs\` or \`install\`/ \`install-all\`/\`reinstall\`, so the previous filter stayed false and the ordering between prepare-gems and ext/configure-ext.mk regen was not enforced -- a clean-tree parallel docs install could still race. Add \`docs install install-all install-doc install-html install-rdoc reinstall\` to the filter. For \`make install INSTALLDOC=nodoc\`, prepare-gems was already pulled in transitively via pre-install-gem, so this only adds an ordering constraint, not a new fetch. Verified with \`make -n\` on a clean tree: plain \`make\` does not extract gems; \`make docs\`, \`make install\`, and \`make html\` do. https://claude.ai/code/session_01YRoRyZPew2LtN6u6BX8bzx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aad8a06bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # docs goals. Other targets are left alone so a plain `make` doesn't | ||
| # reach for prepare-gems -> update-gems (network). | ||
| ifneq ($(filter rdoc rdoc:% html html-server rdoc-coverage undocumented docs install install-all install-doc install-html install-rdoc reinstall,$(MAKECMDGOALS)),) | ||
| ext/configure-ext.mk: $(HAVE_BASERUBY:yes=prepare-gems) |
There was a problem hiding this comment.
When the docs workflow invokes make XRUBY=ruby RDOC_DEPENDS= RBCONFIG=update-rbconfig ... rdoc-coverage/html (checked in .github/workflows/check_misc.yml), the html and rdoc-coverage targets no longer depend on main/srcs-doc, so ext/configure-ext.mk is not requested at all. Adding prepare-gems only as a prerequisite of ext/configure-ext.mk therefore never runs in that CI path, leaving a clean tree able to load rdoc/rdoc before the bundled rbs gem is extracted and built—the failure this change is meant to fix still occurs for the documented workflow.
Useful? React with 👍 / 👎.
2 participants
Why
After b5e6e0a (
Use rbs-integrated RDoc), the dailydocs.ymlworkflow fails on a clean tree.tool/rdoc-srcdirdoesrequire 'rdoc/rdoc', RubyGems tries to activaterdoc-7.2.0's newadd_dependency 'rbs', '>= 4.0.0', and aborts:Root cause
ext/configure-ext.mkis regenerated fromtemplate/configure-ext.mk.tmpl, which enumerates bundled-gem extensions withIts existing prerequisites —
$(PREP) all-incs $(MKFILES) $(RBCONFIG) $(LIBRUBY) template/configure-ext.mk.tmpl update-default-gemspecs— don't include anything that ensures.bundle/gems/is populated. On a clean tree, file timestamps in a failingmake htmlshow:So
exts.mklacks the per-bundled-gem extension build rules,rbs'sextconf.rbis never invoked, no.bundle/extensions/.../rbs-4.0.2/gem.build_completemarker is written, and RubyGems then refuses to activate the spec. This was previously latent — nothing in the docs path required a C-extension bundled gem to be activatable until the rdoc bump introduced the runtime dep on rbs.I verified the diagnosis directly: with no source changes,
rm -rf .bundle && make prepare-gems && make htmlsucceeds. Same command without the manualprepare-gemsstep fails.How
Add
prepare-gemsas a prerequisite ofext/configure-ext.mkso the template glob sees the bundled gems. Gated with$(HAVE_BASERUBY:yes=…)— the same patternprepare-gemsitself uses for its subtargets — so the no-baseruby path is preserved.Once the ordering is correct, the existing
ext/extmk.rbmachinery handles the rest: each bundled gem's generated Makefile hasall: installandinstall: gemspec, which both builds the extension (touchinggem.build_complete) and copies.bundled.<gem>.gemspecinto.bundle/specifications/. RubyGems then finds the spec, sees the build marker, and activates rbs normally.Follow-ups (not in this PR)
The C-extension special-case in
tool/lib/bundled_gem.rb:77-85— routing those gemspecs into.bundle/gems/<gem>/and writing a parallel.bundled.<gem>.gemspec— is the structural reasonext/extmk.rbneeds that gemspec-copy rule andtool/outdate-bundled-gems.rbneeds the second existence check. With the ordering fixed it all works, but consolidating bundled-gem specs into.bundle/specifications/directly would let three pieces of duplicated machinery be deleted. Worth a separate PR.https://claude.ai/code/session_01YRoRyZPew2LtN6u6BX8bzx
Generated by Claude Code