Include prerelease gems in compact index during incremental update#108
Include prerelease gems in compact index during incremental update#108sudoremo wants to merge 3 commits intorubygems:mainfrom
Conversation
75bc582 to
52966ab
Compare
|
The latest addition should fix all unit tests. |
|
Please keep the existing The current change loosens the validation by replacing |
|
Thanks @hsbt for your feedback. The requested changes have been implemented. |
| assert_equal <<~VERSIONS_FILE, File.read(File.join(@indexerdir, "versions")) | ||
| #{versions_file.chomp} | ||
| d 2.1 #{file_md5(File.join(@indexerdir, "info", "d"))} | ||
| d 2.1,2.2.a #{file_md5(File.join(@indexerdir, "info", "d"))} |
There was a problem hiding this comment.
Hmm, test is super long and not simple to follow. I'm not 100% sure this is correct, since new version should be on separate line (appended) when modifying existing file to keep it partially cacheable. And compacted later on demand. Not sure this tests appends or does new index from scratch. Are there tests for both case? I can take a look later to understand the tests if needed.
There was a problem hiding this comment.
Good question! This test exercises the incremental update_index path (not a full rebuild). Both d-2.1 (released) and d-2.2.a (prerelease) are added in the same update_index call, so compact_index_gems groups them into a single CompactIndex::Gem, and VersionsFile#contents appends one line per gem per update — hence d 2.1,2.2.a on a single line.
This is consistent with how a full generate_index works too — see test_build_indices where the versions file has d 2.0,2.0.a,2.0.a-x86-linux,2.0.b on a single line.
The info/d file does correctly have each version on its own line (appended to the existing content via update_compact_index at line 524).
So to summarize the test coverage:
- Full rebuild:
test_build_indices/test_generate_index - Incremental update (append):
test_update_index— this is the one changed here
There was a problem hiding this comment.
This is what happens on rubygems.org, in there new gems are appended (until compacting happening each month).
It is to make versions file cacheable at least for one calendar month and you're downloading only increments at that time. If this is testing appending, I assume it should create new line. But it seems this is not new issue with your addition, so maybe ok to accept this for now. I can check in separate PR to add also incremental additions (maybe as an option?).
In `update_index`, the call to `update_compact_index` only received `released` specs, excluding prerelease gems from the compact index files (`versions`, `info/<gemname>`). Since modern Bundler prefers the compact index, prerelease gems were effectively invisible after an incremental `--update`. Fix by passing all specs (both released and prerelease) to `update_compact_index`. Fixes rubygems#48
Replace assert_match/start_with? with assert_equal for the info/d compact index file, keeping full regression coverage as requested in review feedback.
f0f0f83 to
c30dc55
Compare
|
@sudoremo Can you fix test failure at first? |
On Ruby < 3.5.0, prerelease gems include a rubygems:> 1.3.1 dependency in their compact index info line. The assertion for the d-2.2.a prerelease entry added during update_index was missing this version-conditional suffix, causing failures on Ruby 3.0, 3.1, and 3.2.
|
@hsbt Good catch — the test failure was caused by a missing version-conditional suffix on the Fixed in 6428295 by applying the same Could you approve the CI run when you get a chance? Since each run requires maintainer approval, iterating on failures is a bit slow on our end. |
Summary
Fixes #48.
In
update_index, the call toupdate_compact_indexonly receivedreleasedspecs, excluding prerelease gems from the compact index files (versions,info/<gemname>). Sincebuild_compactdefaults totrueand modern Bundler prefers the compact index over legacy specs files, prerelease gems were effectively invisible after an incrementalgem generate_index --update— even though they were correctly added toprerelease_specs.4.8.gz.The fix passes all specs (both released and prerelease) to
update_compact_index, consistent with howbuild_compact_indexalready handles both during a fullgenerate_index.Changes
lib/rubygems/indexer.rb: Passspecsinstead ofreleasedtoupdate_compact_indextest/rubygems/test_gem_indexer.rb: Add assertions for prerelease gemd-2.2.ain compact index afterupdate_index