Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include a Cargo.lock #3788

Open
getchoo opened this issue Nov 20, 2024 · 6 comments · May be fixed by #3789
Open

Include a Cargo.lock #3788

getchoo opened this issue Nov 20, 2024 · 6 comments · May be fixed by #3789
Labels
C-feature Category: feature. This is adding a new feature.

Comments

@getchoo
Copy link

getchoo commented Nov 20, 2024

Is your feature request related to a problem? Please describe.
Without a Cargo.lock, building the hyper crate itself (i.e., as a cdylib for curl) can be difficult to do in a reproducible manner and/or in environments where offline building is required.

Describe the solution you'd like
Cargo.lock being removed from .gitignore and distributed upstream.

Describe alternatives you've considered
Downstream vendors can make their own Cargo.lock files. This is currently being done in NixOS/nixpkgs#357552.

Additional context
Previously, the Cargo team recommended against having Cargo.lock files distributed by libraries. As of last year, they have changed this advice to "recommend people do what is best for their project".

@getchoo getchoo added the C-feature Category: feature. This is adding a new feature. label Nov 20, 2024
milibopp added a commit to milibopp/hyper that referenced this issue Nov 20, 2024
@milibopp milibopp linked a pull request Nov 20, 2024 that will close this issue
@seanmonstar
Copy link
Member

Hm, I think I'd still defer having a lock file checked in. My understanding of the Cargo team's recommendation is that it can help with CI. That hasn't been an issue for me.

It's likely we wouldn't remember to update it frequently. I wouldn't want people who are building it to depend on that file. I'd rather those building it to have their own lock file and control exact dependency versions.

Is that a problem, such as for NixOS?

@milibopp
Copy link

Is that a problem, such as for NixOS?

We prefer relying on upstream for lock files over shipping our own, but it is possible to do have our own and is done in quite a few packages. Nix by design has very strong reproducibility guarantees, so we must lock down dependencies eventually.

If we do that ourselves, we do that later than you as a maintainer when you release a new version. This causes a discrepancy between the known-good dependency tree when you released and whatever we ship later. If everybody follows semantic versioning or the test suite is complete enough to catch potential problems, this should not cause issues, but it could in principle.

I personally think there is value in having a central authority on the lock file, rather than decentralizing that concern towards the various downstream distributions as any dependency bugs only have to be fixed once. In particular, it would be so easy to overlook important patches somewhere in the dependency tree that could be fixed by running cargo update. Besides, users doing custom builds can still ignore the lock file in the repository and generate their own.

@getchoo
Copy link
Author

getchoo commented Nov 20, 2024

My understanding of the Cargo team's recommendation is that it can help with CI.

I feel this is a bit of a simplification. From the Cargo book FAQ:

The purpose of a Cargo.lock lockfile is to describe the state of the world at the time of a successful build. Cargo uses the lockfile to provide deterministic builds at different times and on different systems, by ensuring that the exact same dependencies and versions are used as when the Cargo.lock file was originally generated.

This may help in CI, but it has a much greater benefit in environments not regularly accounted for, such as those that would usually ship much older or extremely new versions of libraries.

I would also like to put a lot of emphasis on @milibopp's point above:

I personally think there is value in having a central authority on the lock file, rather than decentralizing that concern towards the various downstream distributions as any dependency bugs only have to be fixed once.

If hyper is to ever become a common backend for curl among distributions, this would create a lot of needlessly duplicated work across the entire ecosystem -- putting yet another obstacle in the way of adoption. Of course this is by no means a dealbreaker as packagers have adapted to these kinds of cases over the years, but can create some friction as explained here and above

@JohnRTitor
Copy link

It's likely we wouldn't remember to update it frequently

A CI job could be setup to update cargo locks automatically, or perhaps a pre commit hook that runs cargo update.

Former is better I believe.

@lu-zero
Copy link

lu-zero commented Nov 21, 2024

IMHO having the Cargo.lock in git is an endless source of churn for the developers with little gains for the distributors if the releases are frequent enough.

If the releases are infrequent distributions HAVE to bump the dependencies on their own.

Having the release process provide a known good Cargo.lock is enough for all the intended purposes.

@stephank
Copy link
Contributor

If there is any intention to at some point provide official dylibs for the C API (as opposed to relying on external / distribution packaging), then a Cargo.lock is going to be required. Might be useful for a redistributable Windows DLL or macOS / iOS framework, but feels like it'd primarily benefit proprietary stuff.

At the same time, maintaining a redistributable means there may have to be Hyper releases just to update Cargo.lock, which don't make much sense for consumers of the Rust crate. (Though that could use some subversioning scheme.)

Either way, that burden is going to end up somewhere. Either Hyper does it or distributors have to do it. And I think it's important to highlight that it's not just compatibility testing that has to happen at that point, but also monitoring for security issues in dependencies. Those resources may only be available to larger Linux distributions or larger corps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants