Replace crates-index with tame-index
#506
Conversation
|
Oh, didn't realize this crate tested in several old versions, I'll relax it to 1.67.0. |
|
This was fixed in EmbarkStudios@a966a4e, not sure why the commit isn't being picked up in this PR... |
Another contributor in another repo reported the same problem yesterday. I have to assume this is a GitHub bug. A retry like |
There was a problem hiding this comment.
This seems very nice! I like many of the changes you've decided to make relative to crates-index and I'm excited to merge this.
I flagged one concern that I believe should be possible to resolve without too much additional trouble.
Also, one more question: would this PR also enable our users to use mirrors of crates.io via source replacement? That's been a commonly requested feature (#160) and I was wondering if this PR might also happen to resolve that as well.
For source replacement of crates.io, yes, this PR resolves that (IndexUrl::crates_io), however #160 could mean both source replacement of crates.io or custom registries in addition to or instead of crates.io, which this PR alone wouldn't resolve, as each unique index would need to be opened based on the set of unique source indices, similar to what this code is doing in cargo-deny. |
|
The code looks good to me! If you could try to amend your latest commit and force-push it to work around this GitHub glitch we seem to be having, I'm happy to merge as soon as CI passes. Thanks again for letting me know about your fork of If you might be open to adding support for custom registries at any point, I'd love to have your help on that as well! |
|
Sorry should have been more clear, the 1.67.0 problem was because of rust-version is fixed, however there is a legitimate bug in the current code that my testing didn't catch because of some peculiarities with cargo, fixing that now before rerunning the tests here. |
|
Ok, so the error in https://github.com/obi1kenobi/cargo-semver-checks/actions/runs/5671264307/job/15368054854?pr=506 is resolved at the head of the branch I patched to since I could repro the CI environment locally in a container, the issue seems to be that it is using a cached binary where the cache key isn't accounting for Cargo.lock changes, so not sure how to fix that, but I'll publish the fixed version and change this PR to use that published version. |
|
Oh, and this PR broke the 1.64.0 check, I thought this crate only cared about 1.65.0 so set the rust-version to that, I'll update it to 1.64.0 as otherwise that job will succeed, but by failing on an error that bypasses the actual failure that job is intended to test. |
|
Oh wait, furthest tame-index can go back is 1.65.0, which is the lowest version supported by gix https://github.com/search?q=repo%3AByron%2Fgitoxide%20rust-version&type=code |
|
Ahh, so nvm looking how the caching works the cache key is correct and it is using the correct version, I'll continue taking a look tomorrow. |
That check was actually buggy in itself, fixed in #507 — thanks for getting me to take a look 😅 The actual min supported version is 1.67 and we don't need support for anything older than that.
So supporting 1.67 in
Yeah, as you probably already saw, we just build the binary once in the workflow (per-run) and reuse it in multiple jobs to skip needing to compile from scratch every time. The failure is worth looking into, though no particular rush -- tomorrow is fine. Thanks for digging into this! |
This PR makes a big change to how the RemoteGitIndex performs fetches (and clones) and subsequently reads blobs, to resolve issues discovered while doing obi1kenobi/cargo-semver-checks#506 Previously when performing a fetch, we also updated references so that HEAD pointed at the same commit as the remote HEAD. This works fine in local operation and in tests, but had a drawback, namely ``` Error: Failed to update references to their new position to match their remote locations Caused by: 0: The reflog could not be created or updated 1: reflog messages need a committer which isn't set ``` can occur in CI environments. My attempts to fix this...failed (configuring a committer name + email before editing references). Then I noticed frewsxcv/rust-crates-index#129 which....is just way better than that previous approach since we don't need to edit references any longer just to be able to use `repo.head_commit` for reading blobs, but rather just store the remote HEAD when opening/fetching and retrieving the tree to lookup blobs from that, sidestepping the whole issue with updating references. There is a big difference between that PR though, in that this PR also adds a function (`crate::utils::git::write_fetch_head`) to actually write a FETCH_HEAD _similarly_ to how cargo, via git or libgit2 writes FETCH_HEAD when performing a fetch on an index. This was done for 2 reasons: 1. Performing a fetch of an index via this crate will now update the index repository the same as cargo would, which makes this play nicer with cargo and thus the wider ecosystem 2. I had already done (a slightly worse) version of writing a FETCH_HEAD for [cargo-deny](https://github.com/EmbarkStudios/cargo-deny/blob/main/src/advisories/helpers/db.rs#L451-L457), because, AFAIK, retrieving the timestamp of the FETCH_HEAD file is by far the [most/only reliable](https://stackoverflow.com/questions/2993902/how-do-i-check-the-date-and-time-of-the-latest-git-pull-that-was-executed) way to determine when a fetch was last performed on a repository. The reason this was important for cargo deny is that it fetches advisory databases, but can decide _not_ to fetch them if the user doesn't want to perform network calls, however if they _never_ fetch from the remote advisory db they run the risk of mistakenly thinking everything is fine even if they have crate dependencies that have active advisories that were added/modified since the last time they fetched. So until `gix` adds support for writing FETCH_HEAD (which I think is planned, but couldn't find it), making this function public allows cargo-deny or others to also have a (simple, definitely not git/libgit2 compliant) way to write a FETCH_HEAD with gix.
|
Ok, I think this is ready to go now, doing this PR forced me to fix several bugs and add improvements to tame-index, eg EmbarkStudios/tame-index#7, so it's now in a much better state. |
|
If you'd like, I can also do a follow up PR to replace git2 with gix to reduce compile times/binary size. |
|
This looks great, thanks so much for digging into the issues and fixing them! I'm very excited to merge and release this 🚀 Is my understanding correct that this PR switches our index behavior from "always git index" to "same as cargo" i.e. default-sparse unless configured otherwise? If so, then I think all our CI tests are now over a sparse index and we have a gap in our test suite. Would you mind adding a test job that makes sure semver-checking with a git index still works as expected? For example, perhaps duplicate the |
|
The |
|
Unfortunately, that is only temporary coverage — this next release is the last one that will support 1.67 — so that job will get updated to use two newer Rust versions that happen to emit different rustdoc JSON formats. I'd really love an explicit test job for testing the git index, because bugs related to index configuration are always such a pain to triage and debug. How can I make adding that test as easy as possible for you? I've never added or modified index configs before, so I don't know how to assert that cargo / cargo-semver-checks are using a git index. But if you'd prefer, I can add two new jobs (Linux / Windows) that catch semver issues and have a fill-in-the-blank step that should add the configuration and assert it's being applied correctly. |
|
Oh it's no problem for me to add them I just thought I would point out there is still at least one test using the git index, I'll add variations for the above tests you recommended. |
|
Thanks, I really appreciate it! Also, switching to |
|
Very interesting, TIL about that env var! Is I'm a bit worried about "test passes for the wrong reasons" situations, like accidentally ignoring the env var and falling back to the default index config. |
|
Yes, the purpose of the |
|
Thanks again! If you're still interested in porting |
|
Next week ;) |
|
Enjoy your weekend! |
This PR replaced
crates-indexwithtame-indexwhich gives more friendly access to the crates.io index, whether it is git, sparse, or source replaced with another git or sparse registry.Resolves: #487