search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.76k stars 2.42k forks source link

Tracking Issue for `-Zgitoxide` #11813

Open Byron opened 1 year ago

Byron commented 1 year ago

Summary

RFC: None Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#gitoxide Issues: https://github.com/rust-lang/cargo/labels/Z-gitoxide Original issue:

With the ‘gitoxide’ unstable feature, all of the specified git operations will be performed by the gitoxide crate instead of git2. This allows git2 to be replaced in full.

Steps

Unstable feature sub-categories:

Further tasks

Unresolved Issues

Future Extensions

Some features not in libgit2 are good candidates for future extensions

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Implementation history

epage commented 1 year ago

transitive dependency tempfile is pinned to ~3.3.0 temporarily (see gix-tempfile@4.1.1)

This surprises me and the links don't quite make the motivation for this clear.

From my experience

weihanglo commented 1 year ago

Hmm… agreed! gix-tempile previously depends on 3.4.0, so alternatively it could just depend on ^3.3.0 and we handle it in rust-lang/rust. I cannot guarantee but believe bootstrap team will fix it in the near future :).

ehuss commented 1 year ago

I would agree that using ^3.3.0 would be the more appropriate requirement. I would generally avoid ~ in most situations.

I cannot guarantee but believe bootstrap team will fix it in the near future

I do not think anyone is looking at fixing this particular issue. The fix from https://github.com/rust-lang/rust/pull/106610 didn't seem to work for me, though I'm not sure why.

If cargo moves to a separate workspace, then this particular problem might not be an issue.

Byron commented 1 year ago

I don't know if there is a better solution to this problem than the one we opted for, but by sharing the status quo maybe we will come up with something.

When publishing gix-tempfile v4.1.1 my CI broke (due to cargo deny) and for some reason I didn't even notice it. Had I known that there is a RUSTSEC advisory out aganst tempfile 3.3 I would not have pinned to it so lightheartedly.

Since then, I have removed the pin of tempfile in such a way that the next release of gix-tempfile will be a major version change to fix my CI. gix-tempfile v4.1.1 will still cause other peoples CI to fail for the same reason it failed for me, and it's pulled in by default until it is yanked by all available versions of gix.

Soon I hope to be able to publish a new major version of gix which then depends on gix-tempfile v5.0, providing an upgrade path out of this situation.

Thus far, doing this was quite some churn and already caused breakage in other people's CI, and there is probably some fallout that I don't know about. The main culprit here was me being unaware of the security advisory which is sure way to break people who run cargo deny. I am sure there are plenty of other ways as well even without that.

It might be worth investigating a better process to handle such issues as I have a feeling the probability for this to happen is quite high given the large amount of additional dependencies that gitoxide (still) pulls in combined with the large amount of platforms cargo is being compiled for.

epage commented 1 year ago

When publishing gix-tempfile v4.1.1 my CI broke (due to cargo deny) and for some reason I didn't even notice it. Had I known that there is a RUSTSEC advisory out aganst tempfile 3.3 I would not have pinned to it so lightheartedly.

This is not the only reason to avoid pinning in version requirements. See my previous link where another crate author pinned things and caused problems.

Since then, I have removed the pin of tempfile in such a way that the next release of gix-tempfile will be a major version change to fix my CI.

Why did removing the ~ require a major version change? It seems like a ^3.3 could be in a 4.1.2

Byron commented 1 year ago

This is not the only reason to avoid pinning in version requirements. See my previous link where another crate author pinned things and caused problems.

Even though I didn't acknowledge it I don't think I made the claim that what happened here is all that can happen. Am I missing another point?

Why did removing the ~ require a major version change? It seems like a ^3.3 could be in a 4.1.2

As of now, cargo seems to require the dependency fix/hack to function. I made my CI fix (removing the pin) breaking to assure that the next release will be a major one that cargo update will not automatically update to, which would undo the fix. Now, gix up to v0.40 will use the pinned version, and gix v0.41 which I am prepping now will put things back to normal.

Funnily enough I am also preparing a PR that upgrades cargo to gix v0.41 to get a fix for this issue, so I guess that one will be blocked until the underlying cargo issue is fixed. 'Kerfuffle' is my favorite word for this, right after 'conundrum' 😅.

epage commented 1 year ago

Even though I didn't acknowledge it I don't think I made the claim that what happened here is all that can happen. Am I missing another point?

I was just wanting to confirm since I had read your comment as the RUSTSEC issue being the only thing that would have had you re-evaluate.

As of now, cargo seems to require the dependency fix/hack to function. I made my CI fix (removing the pin) breaking to assure that the next release will be a major one that cargo update will not automatically update to, which would undo the fix

Maybe others feel different but I would not consider this to be a part of your project's compatibility guarantees and personally feel making it a breaking change exacerbates the problem as the max version (default for the resolver) in the 4.x.y series does not play well with other crates.

Funnily enough I am also preparing a PR that upgrades cargo to gix v0.41 to get a fix for https://github.com/rust-lang/cargo/issues/11821, so I guess that one will be blocked until the underlying cargo issue is fixed. 'Kerfuffle' is my favorite word for this, right after 'conundrum' 😅.

Why is this necessarily blocked? Couldn't gix-tempfile use ^3.3 instead of ^3.4? Then cargo could use 3.3 and everyone else can avoid the tainted version and use 3.4.

Byron commented 1 year ago

I was just wanting to confirm since I had read your comment as the RUSTSEC issue being the only thing that would have had you re-evaluate.

Right! Even though I ran into issues like the one in your link myself and know that pinning is usually going to bite, in this case I didn't think much about the possible repercussions as fixing cargo seemed like an overwhelming priority along with a lack of alternatives.

Maybe others feel different but I would not consider this to be a part of your project's compatibility guarantees and personally feel making it a breaking change exacerbates the problem as the max version (default for the resolver) in the 4.x.y series does not play well with other crates.

Definitely, it breaks the stability guarantee of the project as per STABILITY.md (even though it doesn't actually break anybody), and the fix for the v4.x. line of gix-tempfile will happen by yanking v4.1.1 once cargo is in the clear. It is a hack to protect another hack which seemed inevitable.

Why is this necessarily blocked? Couldn't gix-tempfile use ^3.3 instead of ^3.4? Then cargo could use 3.3 and everyone else can avoid the tainted version and use 3.4.

How would cargo be able to force itself to use gix-tempfile v4.1.1? Refer to the crate and pin it maybe using ~4.1.1? Right now the assumption is that it is served whatever gix comes with, and that will currently top out at 4.1.1 by normal resolution rules.

Probably I was missing the obvious solution of pinning gix-tempfile in cargo, as now what happened is said exacerbation of the problem. gix v0.41 now refers gix-tempfile v5.0 which is good technically but wrong from cargo, which now cannot pin a lower version anymore without conflicts (or at least, it wouldn't force the whole dependency tree to use the same version). But if that were true, couldn't cargo have pinned tempfile all by itself in the first place?

So it looks like all that can be done to fix this issue is to have one PR which disables the tests in question, and another that re-enables them and upgrades to gix v0.41 that contains the actual fix and removes the pinned tempfile crate. And then we hope cargo and tempfile v3.4 will soon work again so the hacks can be removed as good as possible.

epage commented 1 year ago

Definitely, it breaks the stability guarantee of the project as per STABILITY.md (even though it doesn't actually break anybody

I did not see anything that said you are responsible for bugs in your dependencies. tempfile didn't see a reason to make it a breaking version, so what isn't clear to me is why it needs to be one for gix-tempfile.

Also, unsure if we've already had this conversation but I would recommend reconsidering MSRV changes being breaking changes. I know that was a topic for a while during the early days of MSRV discussions but I feel like things have mostly settled towards not doing that. Take the recent libc MSRV discussion. That had a lot of voices with a lot of different (strong) opinions but only 1 person advocated for a general policy. The only other mention of it I found was Josh bringing it as part of brainsxtorming for breaking the gordian knot for libc. Updating MSRV is like rustc updating the kernel, glibc, or android NDK.

semver is a tool to help and if it is used to cover too much ground it can make it difficult to make any progress or can cause extra churn from too many breaking releases and then hurts more than helps. I originally wrote this in talking about MSRV but I suspect it ties into our tempfile conversation

How would cargo be able to force itself to use gix-tempfile v4.1.1? Refer to the crate and pin it maybe using ~4.1.1? Right now the assumption is that it is served whatever gix comes with, and that will currently top out at 4.1.1 by normal resolution rules.

Unsure if this is what you are getting at but something I had overlooked is the challenge of (some) monorepo development: intra-repo version requirements. I'm assuming on every release all dependents get their version requirement updated to latest. This makes development in the repo easier but increases churn for end-users and offers users fewer workarounds, like in this case. I say this not to say what is the right approach for gix but to acknowledge it. gix is of the scale it could be hard to manage. In some of my projects, I do similar though in ones like clap where its smaller and the pieces are less entangled, I don't. One future possibility to help with this is my idea to stablize the #[stable] attribute and have clippy or rustc produce warnings when you use an API item from newer than the minimal version of your version requirement. While it wouldn't guarantee that version reqs within the repo are always valid, it helps.

. But if that were true, couldn't cargo have pinned tempfile all by itself in the first place?

Maybe? I don't have the context for what precipitated this. The tracking issue links to a tag which links to a comment from Chris Denton that says nothing about tempfile and I've expanded commetns around it and I don't see anything. I did a search for tempfile on the page and only found this comment from ehuss.

Byron commented 1 year ago

semver is a tool to help and if it is used to cover too much ground it can make it difficult to make any progress or can cause extra churn from too many breaking releases and then hurts more than helps. I originally wrote this in talking about MSRV but I suspect it ties into our tempfile conversation

Making MSRV changes non-breaking definitely can be discussed - while gix is in pre-release mode I find it easy to declare it a breaking change and prevent anybody downstream from being broken by a patch release. For stable releases, I'd love this to change as well even though I see value in the downstream being able to protect itself from breakage without having to pin their gix dependency down to the patch level. gitoxide's MSRV is effectively dictated by the windows dependency, and that makes it even harder to 'do the right thing'. Wasn't there an RFC that would allow cargo to choose only versions that have a compatible MSRV? With that, the problem would be solved I think.

This is not to discard the suggestion of making MSRV changes non-breaking, it's just to explain who one could choose one over the other to prevent downstream breakage. To me, that is still paramount, and ideally there is a better way of doing things once gix goes 1.0 to maintain that major version number for 6 months or more.

[..] One future possibility to help with this is my idea to stablize the #[stable] attribute and have clippy or rustc produce warnings when you use an API item from newer than the minimal version of your version requirement. While it wouldn't guarantee that version reqs within the repo are always valid, it helps.

On the question on managing complex workspaces with plenty of interconnected crates, having #[stable] would definitely be another tool in the belt. It would probably be particularly useful for stable crates (i.e. X.Y with X>0).

Maybe? I don't have the context for what precipitated this. The tracking issue links to a tag which links to a comment from Chris Denton that says nothing about tempfile and I've expanded commetns around it and I don't see anything. I did a search for tempfile on the page and only found https://github.com/rust-lang/rust/pull/108665#issuecomment-1452337751.

Seeing this makes me fear I made all this up due to some huge misunderstanding 😅. The issue feels quite illusive, too, and I guess it's time for some sharing my take-aways:

This whole topic did costs me a couple of hours and certainly had higher costs throughout the ecosystem due to gix-tempfile being tainted for a day without alternative. I will wait for the tempfile and cargo issues to be resolved (in the hopes of being pinged when that happens) to yank the tainted version and allow upgrades to more recent gix versions (> v0.40) to happen.

epage commented 1 year ago

Wasn't there an RFC that would allow cargo to choose only versions that have a compatible MSRV? With that, the problem would be solved I think.

It was discussed as a possibility as part of package.rust-version. We still need to work out the UX once we have package.rust-version in the index. This will help a lot, along with a list of other ideas for improving the experience summarized in a post I made in the libc thread that is now in the hidden section, making it difficult to get to...

Seeing this makes me fear I made all this up due to some huge misunderstanding 😅. The issue feels quite illusive, too, and I guess it's time for some sharing my take-aways:

Seems like we are on the same page now. Unsure what to do about whats been done but hopefully we won't hit this again. I'll go ahead and close these comments out as resolved.

Nemo157 commented 11 months ago

Is it possible to enable this through environment variables somehow? I wasn't able to figure out any syntax that didn't run into this error:

> CARGO_UNSTABLE_GITOXIDE=true cargo build
error: missing field `fetch`

(it may also be useful for this usecase to support an explicit CARGO_UNSTABLE_GITOXIDE=all feature alias that acts like -Zgitoxide)

Byron commented 11 months ago

I think I did that with the following, on stable:

export CARGO_REGISTRIES_CRATES_IO_PROTOCOL=git
export __CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2=1 # nobody should be using this on stable, but it's there if truly needed.

CARGO_UNSTABLE_GITOXIDE=fetch cargo build should work, and so should CARGO_UNSTABLE_GITOXIDE=fetch,shallow cargo build for shallow clones. Please note that I wasn't aware of these environment variables before reading this, so I just assume whatever -Z gitoxide=<value> supports can be used as CARGO_UNSTABLE_GITOXIDE=<value>.

Nemo157 commented 11 months ago

CARGO_UNSTABLE_GITOXIDE=fetch cargo build should work

It doesn't, it gives the same error message. The CARGO_UNSTABLE_* environment variables correspond to the [unstable] table in .cargo/config.toml not the -Z flag.

By actually trying to set it in .cargo/config.toml it gave some more useful error messages, and I managed to get it working with

[unstable.gitoxide]
fetch = true
shallow_index = true
shallow_deps = true
checkout = true
internal_use_git2 = false

and 

> CARGO_UNSTABLE_GITOXIDE_FETCH=true CARGO_UNSTABLE_GITOXIDE_SHALLOW_INDEX=true CARGO_UNSTABLE_GITOXIDE_SHALLOW_DEPS=true CARGO_UNSTABLE_GITOXIDE_CHECKOUT=true CARGO_UNSTABLE_GITOXIDE_INTERNAL_USE_GIT2=false cargo build

I do think it would be good to override the toml parser for this variable too so that you don't have to specify every single field.

ibraheemdev commented 5 months ago

How is shallow cloning dependencies different than specifying the refspec (e.g. +HEAD:refs/remotes/origin/HEAD) that cargo already does? I think it only really helps if when fetching a specific branch or tag?

Nemo157 commented 5 months ago

A refspec reduces the heads that you're requesting, so you don't get other unneeded branches being pulled down. But you still request the entire history from that head back to initial commit(s) of the repo. A shallow clone allows you to limit how much of the history you request, presumably for cargo's usecase it only needs to pull down the current commit of the requested head and can ignore the history entirely (though technically this is visible to build.rs scripts running from git dependencies).