Add content addressable gems support - #9773
Conversation
4a5def3 to
d849a56
Compare
ce7e319 to
8d050c3
Compare
tenderlove
left a comment
tenderlove
left a comment
There was a problem hiding this comment.
I didn't review the specs super closely, I trust they are covering useful scenarios.
It looks like we've got a lot of array / hash manipulation going on in this PR. Should we be thinking about making real, named objects?
The direction looks good here IMO
|
|
||
| def hash | ||
| @set.hash ^ @name.hash ^ @version.hash ^ @platform.hash | ||
| @set.hash ^ @name.hash ^ @version.hash ^ @platform.hash ^ @content_address.hash |
There was a problem hiding this comment.
No impact on this PR, but this is a bad hash and we should fix it upstream.
We should be doing [@set, @name, ...].hash
|
|
||
| def hash # :nodoc: | ||
| name.hash ^ version.hash | ||
| [name, version, platform, content_address].hash |
| end | ||
|
|
||
| def spec_platforms(entry, platforms) | ||
| platforms = platforms.transform_values(&:uniq) |
There was a problem hiding this comment.
This is because it's now a hash of hashes? What is the structure of platforms?
There was a problem hiding this comment.
platforms comes from line 213 in output_versions. It's a one-level hash
platforms = Hash.new {|h,version| h[version] = [] }
In reality it would look something like this:
{
Gem::Version.new("1.0.0") => [
Gem::Platform::RUBY,
Gem::Platform.new("x86_64-linux"),
Gem::Platform.new("x86_64-linux")
],
Gem::Version.new("0.9.0") => [
Gem::Platform::RUBY
]
}
And then `transform_values(&:uniq) would remove any dups in the values.
|
|
||
| Gem::NameTuple.new(name, version, platform || "ruby") | ||
| suffix ||= "ruby" | ||
| content_address = suffix if Gem::ContentAddress.match?(suffix) |
There was a problem hiding this comment.
This logic is because we're using this same loop with the CA and non-CA RubyGems endpoints?
There was a problem hiding this comment.
Yep. The compact-index /versions response uses the same suffix field for both formats, legacy entries contain a platform, while CA entries contain a content-address token. This handles both, so it detects CA suffixes and stores them as content_address. The actual platform is decoded later from the gem’s /info metadata!
| platform: platform, | ||
| ruby_abi: ruby_abi_from(requirements[:ruby]), | ||
| } | ||
| end |
There was a problem hiding this comment.
It feels like we should make a real object here. Just spitballing but like:
class GemInfo < Struct.new(:version, :suffix, :platform, :ruby_abi)
def hash; suffix; end
def eql?(other); other.version == version && other.suffix == suffix; end
endThough now that I type this out, it seems very similar to NameTuple? It feels like we could be doing more simple code here with set intersections. e.g. wanted_rows.map { make_obj(_1) } & compact_index_info_rows(name).map { make_object(_1) }
There was a problem hiding this comment.
@tenderlove have implemented a solution in this commit, interested in your thoughts!
2b193ae to
467a7a5
Compare
TestingWith: https://rubygems.org/gems/content_addressable_test 1.1
|
6dc3207 to
f854b32
Compare
Replace Object#present? (ActiveSupport) with a plain truthy check on content_address in CompactIndex::GemVersionMethods, so the vendored lib/compact_index* files no longer rely on Rails being loaded. content_address is either nil or a non-empty hex string (the Version model's CONTENT_ADDRESS_FORMAT validation rejects empty/other values, allow_nil: true), so a truthy check is equivalent to present? here. Ref: ruby/rubygems#9773 (comment)
Added a follow up issue to explore refactoring away the hashes and arrays #9834 - but it's not a blocker for this initial support as we discussed privately. cc: @tenderlove |
27472f4 to
8b25dd2
Compare
|
@hsbt thank you for your eyes on this. Added initial patches of 1, 3, and 4 at the moment. Still working on 2 Shopify#195. 1. Skinny gems do not carry rubygems:>= 4.1.0, so older clients accept them through the local pathThe server actually adds the constraint when pushed https://github.com/rubygems/rubygems.org/blob/master/app/models/version.rb#L502 (also see https://rubygems.org/info/content_addressable_test). But that doesn't cover the local path and it would be better to have this during build instead of modifying it in the server. Added the 3. A CA plugin stub loads ABI-specific code on an older Ruby sharing that GEM_HOMEAdded a subdirectory for Ruby ABI. When a new version of the gem gets installed, removing and regenerating the plugin includes cleaning up plugins in the Ruby ABI subdirectory as well as root. Shopify#192 4. A lockfile written by 4.1 breaks Bundler 4.0, during the co-publication periodRight, the proposed format will not break. The one minor detail is for the checksums, if the skinny binary SHA is recorded, a mismatch SHA error will raise since it'll compare with the fat binary SHA when RubyGems is downgraded. I opted for recording the skinny binary SHA in the new section and recording the fat gem SHA in the original checksums section so if RubyGems gets downgraded, clients will seamlessly use the fat binary SHA instead. Let me know what you think about that, maybe it's fine to store the skinny binary sha in checksums section? 🤔 If you have the code-level comments handy, I would like to know so those can be addressed as well! |
|
Your checksum split looks right to me. Having the content-addressable build's SHA sit in Anything an older client can see has to describe the artifact it actually installs, so the split has to land somewhere, and this is the cheaper place. I would keep it as you have it. |
hsbt
left a comment
hsbt
left a comment
There was a problem hiding this comment.
I have put the rest of my code-level notes on the lines they affect. Three of them could not be anchored to a line, so they are here.
A failed skinny download caches a different artifact under the skinny name. lib/rubygems/remote_fetcher.rb:156 re-raises when spec.original_platform == spec.platform, but original_platform is a String and platform is a Gem::Platform, so the comparison is always false and the guard never fires. For an ordinary gem that is harmless, because the alternate name it falls through to is the same file. For a content-addressed gem the alternate name is foo-1.0-x86_64-linux.gem, a different artifact, and it is written to cache/foo-1.0-deadbeef.gem. The unless File.exist? guard then prevents any refetch, so a transient 404 pins the wrong file under the content address indefinitely. The file is not part of this PR, so I could not comment on the line.
Not implemented yet. The suffix widening path from the RFC is absent. Gem::ContentAddress::PATTERN accepts 8 to 64 characters, but nothing takes a widened name from the server and nothing removes a locally built default-length copy by comparing checksums. gem build also does not print the platform and Ruby ABI of each gem it produces, or emit the manifest the RFC describes. The query commands do decode.
Test coverage. spec/install/gemfile/content_addressable_spec.rb covers coexistence, a matching skinny, a non-matching skinny, the no-skinny fallback, and the skinny-only CHECKSUMS case. There is no example pinning that a pre-4.1 client never sees a content-addressed row, which is the property the required_rubygems_version injection now provides.
| path = @gem&.path | ||
| return unless path | ||
|
|
||
| return nil unless Gem::ContentAddress.applicable?(spec) |
There was a problem hiding this comment.
content_address verifies the filename's suffix against the file's SHA256 just below and raises on mismatch, but this line returns nil without checking anything. Gem::Installer#assign_content_address then assigns that nil over the address the lockfile or index declared, so a disagreement is treated as absence rather than as an error. See my note on installer.rb:976 for what that costs. Failing closed here would cover both halves.
|
|
||
| private | ||
|
|
||
| def assign_content_address |
There was a problem hiding this comment.
This assigns @package.content_address over spec.content_address without comparing them. When the package returns nil, which package.rb:265 does silently, full_name reverts from foo-1.0-deadbeef to foo-1.0-x86_64-linux, so gem_dir now points at the platform gem's directory. The strict_rm_rf gem_dir and strict_rm_rf spec.extension_dir in Bundler::RubyGemsGemInstaller#install then remove an existing, legitimate installation, and because the directory the lockfile expects is never created, the install repeats.
| # content-addressed candidate built for the running Ruby, then the | ||
| # earlier-supplied source. | ||
| candidates.min_by do |s| | ||
| [Gem::Platform.platform_specificity_match(s.platform, Gem::Platform.local), |
There was a problem hiding this comment.
This orders candidates by platform specificity first and the ABI match second, while MatchPlatform.select_best_platform_match filters by content address and ABI first and sorts by platform afterwards. When the platform build is published at a finer granularity than the skinny one, arm64-darwin-27 against arm64-darwin, gem install picks the platform build and bundle install picks the skinny. The RFC calls this ordering a contract both clients must agree on, and its wording of skinny then fat then source reads as ABI first.
| return matching if addressable.empty? | ||
|
|
||
| compatible = addressable.select(&:matches_current_metadata?) | ||
| compatible.any? ? compatible : non_addressable |
There was a problem hiding this comment.
When every candidate is content-addressed and none matches the running Ruby, compatible and non_addressable are both empty and this returns []. Gem::Platform.sort_and_filter_best_platform_match always returns at least one element, so callers rely on non-empty in meaning non-empty out. frozen_bundle_fallback reaches choose_compatible([]), candidates.last is nil, and the version is dropped in Resolver::Package#platform_specs, so the user sees Could not find gem rather than a requires-ruby-version diagnostic. non_addressable.any? ? non_addressable : matching restores the contract.
| suffix = api_data[:suffix] | ||
| required_platform = required_platform_from(api_data.dig(:requirements, :platform)) | ||
|
|
||
| if Gem::ContentAddress.match?(suffix) && required_platform |
There was a problem hiding this comment.
This sets @content_address from the suffix and a platform requirement without consulting the Ruby requirement, so a row lacking ruby: yields a spec that carries an address while content_addressed? is false, and full_name and file_name disagree.
More generally, there are several conditions in the tree for "is this content addressed" and they do not agree. content_addressed? requires applicable?, LazySpecification#full_name uses match? && platform != RUBY, and thirteen call sites test match? alone. Two of my other comments are instances of the same disagreement, so a single predicate would remove the class rather than its members.
| private | ||
|
|
||
| def plugin_stub_dir_for(spec, plugins_dir) | ||
| ruby_abi = spec.to_spec.ruby_abi if Gem::ContentAddress.match?(spec.content_address) |
There was a problem hiding this comment.
The guard is match?(content_address) but the directory comes from spec.ruby_abi, which is nil unless required_ruby_version is exactly ~> X.Y.0. A content-addressed gem whose requirement is written any other way falls back to the root plugins directory and becomes visible to older RubyGems again. gem build --ruby-abi cannot produce that shape, so it takes a gem from elsewhere, but the fallback is silent where the guarantee is meant to be structural.
| end | ||
|
|
||
| def ruby_abi_plugin_dir_for(spec, plugins_dir) | ||
| ruby_abi = spec.to_spec.ruby_abi if Gem::ContentAddress.match?(spec.content_address) |
There was a problem hiding this comment.
For a non-content-addressed spec this resolves to the running Ruby's ABI directory, so remove_plugins_for clears the root and the current ABI only and a stub written under a different ABI survives. Install a content-addressed gem under 3.4, then a newer platform build under 4.0, and running under 3.4 loads both plugins/nokogiri_plugin.rb and plugins/3.4/nokogiri_plugin.rb. I confirmed both run. test_load_plugins_loads_latest_non_content_addressed_plugin_after_content_addressed_plugin pins newest-wins within one ABI, and that does not hold across ABIs, which is the shared GEM_HOME case.
|
|
||
| def add_content_addresses | ||
| content_addresses = definition.resolve.filter_map do |spec| | ||
| next unless Gem::ContentAddress.match?(spec.content_address) |
There was a problem hiding this comment.
This writes a line for any spec whose address matches, using spec.lock_name, while NAME_VERSION_CONTENT_ADDRESS requires a platform. A content-addressed spec on the ruby platform produces nokogiri (1.19.4) 86e5e59f, which the parser does not match, so the address is lost on the next read. LazySpecification#full_name already uses match? && platform != RUBY, and the same condition here would keep the two symmetric.
de55806 to
38d2902
Compare
|
@jenshenny The 19 failing Bundler jobs are not from your changes. The branch dropped I verified the rest locally on the current head and it looks right. Suffix widening is the only RFC item still missing, and beta2 seems fine for it. I think this is good to merge once CI is green, so let me know when it is. |
Co-authored-by: Jenny Shen <jenny.shen@shopify.com>
390b9f8 to
f4b5be8
Compare
Co-authored-by: Harriet Oughton <harriet.oughton@shopify.com> Co-authored-by: Jenny Shen <jenny.shen@shopify.com>
Co-authored-by: Gira Chawda <gira.chawda@shopify.com> Co-authored-by: Jenny Shen <jenny.shen@shopify.com>
Co-authored-by: Jenny Shen <jenny.shen@shopify.com>
Co-authored-by: Jenny Shen <jenny.shen@shopify.com>
…al cache Co-authored-by: Jenny Shen <jenny.shen@shopify.com>
Gem::Indexer (rubygems-generate_index) loads the V1-only CompactIndex constants, either from the compact_index gem or from its own embedded copy, so the artifice's copy of rubygems.org's V2-only implementation can never share that name safely. Check in a copy of rubygems.org's lib/compact_index, renamed to the VendoredCompactIndex namespace, under spec/support/vendor/compact_index, and load it from the artifice with a plain require. Checking the copy in (rather than downloading it during the test run) keeps the suite hermetic and offline, makes the namespace rewrite visible in review, and lets parallel workers and CI runners that skip the test-deps setup load it without falling back to the incompatible gem. Refresh the copy with `rake vendor:compact_index`; `rake vendor:compact_index_check` fails if the checked-in copy drifts from the pinned upstream ref.
A content-addressable gem was locked inline as name (version-address)
platform. Bundler 4.0 parses the token after the version as the platform,
gets unknown, materializes name-version-unknown, and fails: it then either
re-resolves and rewrites the lockfile or fails outright under frozen.
Lockfiles are committed and read by many Bundler versions during
co-publication, so the inline form breaks the exact clients the transition
is supposed to protect.
The spec line is now an ordinary platform pin, and the content address moves
to its own section:
GEM
specs:
nokogiri (1.19.4-x86_64-linux)
CONTENT ADDRESSES
nokogiri (1.19.4-x86_64-linux) 86e5e59f sha256=<content-addressable build's sha>
CHECKSUMS
nokogiri (1.19.4-x86_64-linux) sha256=<platform build's sha>
Verified against the released Bundler 4.0.9:
- unknown unindented sections set @parse_method = nil and are skipped
silently (lockfile_parser.rb:137)
- Definition#lockfiles_equal? subtracts unknown sections before comparing
(definition.rb:1196), so a plain 4.0 bundle install does not rewrite the
lockfile and the section survives; it is only dropped on a genuine 4.0
re-lock, and restored on the next 4.1 re-lock
- 4.0 classifies a locked spec with a missing or empty CHECKSUMS entry as a
lockfile change (definition.rb:619-620): a plain install re-resolves and
frozen mode hard-fails. The platform lock name line must therefore carry a
real checksum, and it must be the platform build's, since 4.0 attributes
it to the artifact it installs for that lock name
That last point drives the checksum rules:
- the checksum store is keyed by full name instead of lock name, because a
content-addressable build and the platform build of the same name,
version, and platform share a lock name while being different files.
Store#register accepts anything responding to full_name and lock_name, so
the parser registers CHECKSUMS entries under the name tuple parsed from
the line rather than the (possibly content-addressed) spec object
- the content-addressable build's checksum is serialized next to its
address in CONTENT ADDRESSES, the only place older Bundler never parses,
and verifies the downloaded gem via the existing install-time registration
- CHECKSUMS carries the platform build's checksum, which the compact index
supplies for every row the fetcher sees, so it is captured during
resolution without downloading the platform gem
- when no platform build exists (skinny-only publication), the CHECKSUMS
line is omitted entirely rather than written bare, so every CHECKSUMS
line describes an artifact installable by its lock name
The section registers in SECTIONS_BY_VERSION_INTRODUCED under 4.1.0, and
its line format requires a platform, since content addressing only applies
to platformed gems.
…ute every eligibility, naming, lockfile, plugin directory, and spec construction decision through its shared predicates
Assisted-By: devx/9c2464e2-74cd-4d36-a6c0-50aef8c2ad01
Assisted-By: devx/9c2464e2-74cd-4d36-a6c0-50aef8c2ad01
Assisted-By: devx/9c2464e2-74cd-4d36-a6c0-50aef8c2ad01
3ffc2fb to
f2c0576
Compare
5 participants
#9654
TL;DR
Adds RubyGems and Bundler client support for content-addressable ("skinny") binary gems: one artifact per Ruby ABI, named with a SHA-256 prefix.
The branch covers build, discovery, install, display, yank, lockfiles, caching, and
bundle install --local. Existing source and platform gems are unchanged.Why
"Fat" binary gems contain every supported Ruby ABI and keep growing. Skinny binaries are smaller, but builds for the same gem, version, and platform need distinct filenames. A content-derived suffix gives each artifact a unique identity.
A content address must be 8–64 lowercase hexadecimal characters. RubyGems only treats it as one when the gem also has a non-Ruby platform and constrained
required_ruby_version, avoiding false matches with ordinary filenames.Follow ups
gem buildref: Shopify/rubygems#171.
User behaviour
gem build nokogiri.gemspec --ruby-abi 3.4builds a skinny gem namednokogiri-1.18.9-78be552b.gem, where the suffix is the SHA-256 digest of the gem contents.--ruby-abi, behaviour is unchanged.Details
--ruby-abivalidates the ABI format (X.Y), requires a non-Ruby platform to be set, and constrainsrequired_ruby_version: if unset it defaults to~> X.Y.0; a mismatched existing requirement is rejected.gem installref: Shopify/rubygems#172 (local) and #173 (remote).
Local
gem install --local GEMNAMEis content-addressable aware.Every install of a CA gem writes a gemspec stub whose
# stub:suffix is the hash (so thename-version-<sha>directory resolves). The real platform rides on a separate# stub-target:line that older RubyGems ignore — backwards compatible, while current RubyGems recover both. File:specifications/mygem-1.0-78be552b.gemspec:Remote
platform:=metadata separately. Distinct hashes remain distinct candidates.For example, a server
info/nokogiriresponse with two skinny variants (different Ruby ABIs) plus a platform fallback:The hash is carried in the version token (
1.18.9-78be552b); the real platform and Ruby requirement travel in theplatform:=/ruby:metadata. The two hashes stay distinct resolver candidates, and1.18.9-x86_64-linuxis the platform fallback.gem pushref: Shopify/rubygems#174.
User behaviour
gem push name-*.gem --platform x86_64-linux --ruby-abi 3.4reads the specs of the SHA-named files and pushes the single matching artifact.Details
--platformand--ruby-abiselectors. Given multiple SHA-named files, RubyGems reads each specification and selects the one whose platform andrequired_ruby_versionsatisfy both selectors.ruby_matches?does not check platform (a RUBY-platform gem with a matching~> X.Y.0can be selected by--ruby-abi); this is documented in the tests rather than special-cased.gem yankref: Shopify/rubygems#176.
User behaviour
gem yank mygem -v 1.0.0 --platform x86_64-linux --ruby-abi 3.4sends gem name, version, platform, and ABI so the server can select one skinny variant.--ruby-abi.Remote queries (
gem list/search/info -r)ref: Shopify/rubygems#175.
User behaviour
gem dependencyref: Shopify/rubygems#189.
User behaviour
gem dependency GEM --remotelists content-addressable gems with their content-addressed full name plus the real platform and Ruby ABI, instead of treating the hash as the platform:Details
fetch_remote_specsruns the detected tuples throughGem::SpecFetcher#decode_content_addressable_tuples, which splits the hash from the real platform via the source, fetches each spec, and carriescontent_addressonto it (dependency_command.rb:69-73).content_address_annotationappends(Platform: <plat>, Ruby ABI: <abi>)only whenGem::ContentAddress.content_addressed?(spec)is true (dependency_command.rb:155-165).bundle install(lockfile + local cache)ref: Shopify/rubygems#177 (remote) and #178 (lockfile + local cache).
Remote
Lockfile and local cache
Gemfile.lockand parses both on the next run.vendor/cachewithbundle install --local. Remote and local paths produce the same installed directory; checksums remain keyed by the platform lock name. A CA gem locks with the hash in the version and the real platform beside it.vendor/cache