[Content Addressable] Handle server-widened content address names correctly client-side - #199
Closed
OughtPuts wants to merge 40 commits into
Closed
Conversation
mkmf.log and gem_make.out were written into the installed extension directory, polluting the install tree and breaking bit-for-bit reproducibility checks on distros like Guix and Nix. A successful build now leaves no logs behind, and a failed build writes them to build_info (<full_name>.mkmf.log / <full_name>.gem_make.out) for inspection. https://bugs.ruby-lang.org/issues/21995 ruby#6259 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Clean up the per-gem mkmf.log and gem_make.out left in build_info by a failed extension build when the gem is uninstalled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These specs read the `make -jN` command line from `gem_make.out`, which a successful build no longer writes. The integration specs in `install_spec` now force the build to fail so the command lands in `build_info`, and the `parallel_installer` specs assert on the number of jobserver slots each gem's build acquired, which is exactly what becomes `make -jN`, instead of reading a build log. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Older system RubyGems writes the build log to the extension directory, so under RGV=system the example read the new build_info path and hit ENOENT. The `-j` suppression it verifies only exists with the jobserver support in RubyGems 4.1, so gate it like its sibling examples. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two behaviours regressed against the extension directory the logs used to live in. "clean" is the first make target and mkmf lists mkmf.log in CLEANFILES, so the log was already gone by the time a compile failure reached the handler that moves it to build_info. And nothing cleared a failed build's logs afterwards, where previously the installer wiped the extension directory on every install, so a later successful install kept reporting an old failure. Park mkmf.log next to the built extension right after extconf, the way ExtConfBuilder did before, and let build_extension decide from there whether to drop it or keep it. Drop both logs again on success, along with any gem_make.out an older RubyGems left in the extension directory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
build_info entries are matched by stripping ".info", so the new <full_name>.mkmf.log and <full_name>.gem_make.out never matched an installed gem and were removed as strays, including the log the build error had just told the user to read. Let a subdirectory declare several suffixes and give build_info all three. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Logs for a git source land in bundler/gems/build_info, which `bundle clean` globs as a git checkout and reports as "Removing (build_info)" before deleting it. Exclude it the way the sibling extensions directory already is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Preserving a log happens while a build failure is being reported, so a filesystem error there replaced the compile output the user needed with a bare Errno, and the extension builder stopped raising Gem::Ext::BuildError at all. Callers only rescue the Gem::InstallError family, so the failure escaped as an unhandled exception. Swallow errors from moving mkmf.log and from writing gem_make.out, and drop the "Results logged to" line when there is no log to point at. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An extconf that exits cleanly without generating a Makefile leaves the extension unbuilt while the install still reports success. That case used to be described in gem_make.out; since the success path stopped writing one it went unrecorded entirely. Let the no-Makefile case reach Gem::Ext::Builder, which already owns where logs end up, and have it write the explanation to build_info. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
base_dir for a git source points at the directory holding every checkout, so logs keyed by full_name collided between revisions of the same gem and landed in bundler/gems/build_info, which nothing prunes and which `bundle clean` mistook for a stale checkout. Resolve them from extension_dir instead, which Bundler already makes unique per revision and which `bundle clean` removes along with the checkout. That also drops the clean exclusion added for the directory this no longer creates. Path sources are unaffected: Bundler installs them with extensions disabled, so they never produce a build log. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The git extension spec overwrote the C source after build_git had already committed the checkout, so bundle installed the original working source and the build succeeded, leaving no log to find. Write the broken source inside the build_git block, and assert the log is the one this failure produced. ExtConfBuilder no longer swallows NoMakefileError, so on JRuby, where the fixture extconf returns before creating a Makefile, calling the class method directly now raises instead of returning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Where a build log goes is decided by the RubyGems running the install, and under RGV=system that is an older one which writes a bare gem_make.out into the extension directory. Gate the example the way the other specs that assert on log locations already are. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`bundle version` printed a date that spec/support/build_metadata.rb parsed back out of the top changelog header with a regex, which silently returned nil whenever the header format changed, as it did in 4.0.9. Extract the header rendering into ChangelogHeader so both the changelog cut and the build metadata produce the date from the same `.changelog.yml` template, and pass the stamp explicitly from the spec fixture instead of rewriting a changelog for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RubyGems and Bundler have been versioned in lockstep since 4.0.0 and ship as a single GitHub release, but the release tooling still cut two changelogs from two identical configs and cherry-picked both commits into master. Collapse `.changelog.yml` into one config and split each release section under a `### RubyGems` and a `### Bundler` heading, the way the GitHub release notes have always been split, so a pull request labelled for both libraries is listed under both. The release task now makes one changelog commit and, for patch releases, regenerates the section on master instead of cherry-picking, since the 4.0 branch keeps its per-library changelogs until it is EOL. Cutting keeps every released section in place, replaces the prerelease sections of a final release, and leaves out of a minor release what a library already carried in a lower one, noting that under the library it applies to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Move the 4.0.0 and later sections of CHANGELOG-bundler.md under the `### Bundler` heading of the matching CHANGELOG.md section, leaving every entry as it was published apart from the bullet marker the 4.0.0 to 4.0.8 sections used. The pre-4.0 Bundler history, which used its own version numbers, moves verbatim to doc/CHANGELOG-bundler-2.x.md, and the cross-links between the two files are absolute so they still resolve from inside the gem and from rdoc, neither of which ships doc/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Describe the layout the unified changelog writes, the label rules the release task actually enforces and where they differ by release level, the 4.0 branch that still keeps two changelogs, and the branches a dry run leaves behind for both release paths. Fix the stale label list and the dead link the release guide pointed at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unify the RubyGems and Bundler changelogs
Mapping each commit since the previous release back to a pull request took 102 `gh search prs` calls with a forced sleep every 28 of them, plus one REST fetch per pull request found, about 570 requests and eight to ten minutes for a minor release. `mergeCommit.oid` names the commit a pull request leaves on its base branch under every merge strategy, so one listing call intersected with the local history answers the same question exactly, and without depending on the commit search index. For 4.1.0.beta1 both find the same 466 pull requests, the listing in six seconds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Windows has no `getconf`, so these examples raise Errno::ENOENT on mswin unless MSYS2 or Git for Windows happens to be on PATH. Keep the current PATH as the base there, which also preserves the directories the Ruby under test loads its DLLs from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub reports `mergeCommit` as null for a merged pull request whose merge commit no longer exists, and 190 of the 876 pull requests this repository merged before 2019 are in that state. Reading `["oid"]` off it raised a bare NoMethodError in the middle of a release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reachability test stubbed every backticked command with one canned SHA, so it passed no matter what range `pull_requests_merged_into` asked git for, a reversed one included. It now builds a two commit repository and asserts that only the pull request whose merge commit is inside the range survives. CI checks out with no history, so the range cannot come from this repository. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only the `gh pr list` call checked its exit status. A range git cannot resolve left an empty reachable set that filtered out every pull request, and since nothing raises for a minor release, one would have cut a changelog section with no entries. A tag git cannot resolve surfaced as `invalid xmlschema format: ""` from the date arithmetic instead of naming the ref. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`relevant_pull_requests`, the only caller, already sorts by merge date. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reason a truncated listing is refused was stated on the constant and again above the method that refuses it, and two comments restated their own method signature. The header also claimed the new lookup does not ask the search index, which is wrong: `gh pr list --search` is the same backend as the commit search it replaces, so it inherits the same indexing lag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…b08620 Stop depending on `getconf` in env helper specs
OughtPuts
force-pushed
the
ho/prefer-the-server-widened-identity
branch
from
September 4, 2026 16:05
8a33b2e to
e4f4222
Compare
OughtPuts
marked this pull request as draft
September 4, 2026 16:06
OughtPuts
changed the title
OughtPuts
force-pushed
the
ho/prefer-the-server-widened-identity
branch
from
September 4, 2026 16:41
e4f4222 to
0e39ce7
Compare
OughtPuts
marked this pull request as ready for review
September 4, 2026 16:42
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup can fail for symlinked installation paths and leave stale short-address build metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds client support for server-widened content addresses and safely removes matching short-address installations.
Changes:
- Prefer widened addresses during RubyGems and Bundler resolution.
- Remove identical short-address installations after successful installation.
- Add resolver, installer, integration, and platform-matching tests.
File summaries
| File | Description |
|---|---|
lib/rubygems/resolver.rb |
Prefers matching widened remote addresses. |
lib/rubygems/installer.rb |
Cleans up identical short-address installations. |
lib/bundler/rubygems_gem_installer.rb |
Applies cleanup to Bundler installations. |
lib/bundler/match_platform.rb |
Prefers widened platform candidates. |
test/rubygems/test_gem_resolver.rb |
Tests widened-address resolution. |
test/rubygems/test_gem_installer.rb |
Tests safe installation cleanup. |
spec/install/gemfile/content_addressable_spec.rb |
Adds end-to-end Bundler coverage. |
spec/bundler/match_platform_spec.rb |
Covers differing platforms. |
spec/support/shards.rb |
Assigns the new specification to a shard. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jenshenny
reviewed
Sep 5, 2026
jenshenny
left a comment
jenshenny
left a comment
There was a problem hiding this comment.
Looking good! just dropped a few questions while it's still being polished
Comment on lines
481
to
-483
| installed = candidates.select {|s| s.is_a?(Gem::Resolver::InstalledSpecification) } | ||
| next installed.first if installed.length == 1 | ||
| candidates = installed if installed.any? |
There was a problem hiding this comment.
Hmm does that mean if a gem is already installed, we skip any checks for a widened suffix? Reading the PR description and my mental model, we should check anyways?
Author
There was a problem hiding this comment.
I'm not 100% clear what this means sorry! These checks are covered in the if/else block:
if widened_partially_matching_specs.any?
candidates = widened_partially_matching_specs
else
next installed.first if installed.length == 1
candidates = installed if installed.any?
end
And the .first if .length == 1 side of things is also picked up by candidates.min_by.
Comment on lines
+485
to
+503
| not_installed_widened = candidates.select {|s| !s.is_a?(Gem::Resolver::InstalledSpecification) && s.content_address && s.content_address.length > Gem::ContentAddress::DEFAULT_LENGTH } | ||
| widened_partially_matching_specs = [] | ||
| not_installed_widened.each do |s| | ||
| installed.each do |i| | ||
| next unless s.platform == i.platform | ||
| next if i.content_address&.length != Gem::ContentAddress::DEFAULT_LENGTH | ||
| if s.content_address.start_with?(i.content_address) | ||
| widened_partially_matching_specs << s | ||
| break | ||
| end | ||
| end | ||
| end | ||
|
|
||
| if widened_partially_matching_specs.any? | ||
| candidates = widened_partially_matching_specs | ||
| else | ||
| next installed.first if installed.length == 1 | ||
| candidates = installed if installed.any? | ||
| end |
There was a problem hiding this comment.
The candidate list is only widen specs now, this is unexpected if there are not widened content addresses for different platforms, those candidates are dropped which shouldn't.
Author
There was a problem hiding this comment.
Nice catch, thanks.
I've changed the loop logic in build_spec_for_cache now so that we collect the gems that would be replaced by the widened remote versions, and then remove these from the candidates collection, which will preserve the platformed alternatives.
Comment on lines
+489
to
+493
| next unless s.platform == i.platform | ||
| next if i.content_address&.length != Gem::ContentAddress::DEFAULT_LENGTH | ||
| if s.content_address.start_with?(i.content_address) | ||
| widened_partially_matching_specs << s | ||
| break |
There was a problem hiding this comment.
We should also check if the ruby abi is the same. The only way a widened suffix exists if there's already another gem with a different combo of platform, version number and ruby abi. Replacing a default suffixed gem with a widened one even though they have different ruby abis isn't what we want.
There was a problem hiding this comment.
oh maybe we already filtered by required ruby version by the time we get here?
Author
There was a problem hiding this comment.
We do in filter_specs but seemingly that can be bypassed. I'll add a check just to be extra safe.
jenshenny
force-pushed
the
feature-branch-ca-changes-rubygems
branch
from
September 5, 2026 23:16
3ffc2fb to
f2c0576
Compare
Speed up release preparation by listing merged pull requests
…ubygems Add content addressable gems support
Use separate Bundler download and installation workers
Stop installing native extension build logs
doctor_child took a single extension when the ABI-scoped recursion was written, and gained a splat in the same release, so the recursion has been raising NameError on any repository with a specifications/<abi> directory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The content addressing resolver tests built a "~> X.Y.0" requirement from the running Ruby, which a prerelease Ruby does not satisfy, so both tests failed on 4.1.0dev. Reuse the pinning the dependency installer test already did for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tests feed rubygems something they expect it to reject: two write a gemspec that cannot be loaded, one asks git for a ref that does not exist. Each printed its diagnosis straight into the middle of the test run, as did the rake package task that no longer silenced FileUtils. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SimpleCov writes its summary to stderr, so the redirection meant to keep the collate quiet never caught it and every run ended with the same two lines twice. A failing run replaced the second copy with SimpleCov reporting that it had stopped, which reads like a third failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`2>/dev/null` is a POSIX shell redirect, and backticks reach cmd.exe on Windows, so the two probes that expect to fail started failing for the wrong reason there. Use the array form of IO.popen with `err: IO::NULL`, which the rest of the file already uses for its git probes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The remaining shell redirect had the same portability problem as the two just fixed, and it only escaped Windows CI because no test reaches the release path. Three call sites now share git_quietly, so the next probe has something to reuse instead of a redirect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…se-7589da Fix gem doctor recursion and quiet the test suite output
…cal gem has been installed
OughtPuts
force-pushed
the
ho/prefer-the-server-widened-identity
branch
from
September 7, 2026 13:22
0e39ce7 to
dfbe8a1
Compare
Author
|
@jenshenny I've addressed your feedback and the copilot feedback. Since the feature branch is now merged I've also closed this one off and opened a new PR for the widening work! ruby#9858 |
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
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.
https://github.com/shop/issues-rails-infrastructure/issues/1803
TLDR
Support server-widened content addresses in RubyGems and Bundler.
Summary
When a server provides a widened content address, prefer it over an installed default-length address with the same prefix. After a successful install, remove the short-name copy only when both gem files have identical SHA-256 checksums.
Testing
Top Hat
Install a gem with an 8-character address, then serve the same gem with a widened address. Update it and confirm only the widened installation remains.