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

Add --locked flag to Rust formulae with a Cargo.lock file #46025

Closed
63 of 67 tasks
samford opened this issue Oct 31, 2019 · 7 comments
Closed
63 of 67 tasks

Add --locked flag to Rust formulae with a Cargo.lock file #46025

samford opened this issue Oct 31, 2019 · 7 comments
Labels
outdated PR was locked due to age

Comments

@samford
Copy link
Member

samford commented Oct 31, 2019

The related PR that brought attention to this issue (#45839) is now merged and less visible, so I wanted to create an issue to continue any discussion around the --locked flag, track progress on related formulae updates, and to have something that people can see and reference.

The general idea here is that Rust formulae typically use cargo install to build/install but haven't been using the --locked flag, so builds aren't reproducible and can eventually fail over time if a dependency adds a breaking change in a newer version. In contrast to cargo build, by default cargo install uses the latest dependencies in the build rather than the ones found in Cargo.lock. Adding the --locked flag makes cargo install use the dependency versions in the Cargo.lock file, which is generally what you would expect when you're trying to build a binary of a specific version/revision of a project.

I threw together a little script that goes through the formulae and identifies which have the --locked flag already and which still potentially need it. I've created lists of the formulae 1) with no Cargo.lock file, 2) with a Cargo.lock file now added to the repo but not yet in a release, 3) with a Cargo.lock file but no --locked flag, and 4) with a Cargo.lock file and the --locked flag (i.e., finished).

Feel free to mention this issue in any related PR, as I'll be trying to keep this relatively up to date to track progress.

Does Not Have Cargo.lock

Cargo.lock Added but Not Yet in a Release

Has Cargo.lock and Needs --locked Flag

  • git-series.rb (closed PR: git-series: add --locked flag to cargo install #46176 ----- Build fails due to errors that are fixed in upstream master but not yet in a release; --locked flag will be added in the next release after 0.9.1)
  • rust.rb (Rust and Racer have lock files but not Cargo)

Has Cargo.lock and --locked Flag

@samford
Copy link
Member Author

samford commented Oct 31, 2019

On to questions and comments.

When updating these formulae, if there isn't a new version available to bump the version at the same time, should we bump the revision? That is to say, if we want the bottles to be rebuilt (using the --locked flag) and for users to get an updated binary, does that require bumping the revision? Once this is answered, I can start going through the list in my free time to tackle some of these formulae changes.

I saw that the formula_creator.rb file was updated (Homebrew/brew#6664), so the default cargo install command now contains the --locked flag. The following files in the Homebrew/brew repo may still need to be modified as well:

Per the previous discussion (in the aforementioned PR), it may be necessary to create an audit for the --locked flag (when cargo install is used to build/install). Just to be clear, I don't know enough about Homebrew's internals to take care of this, so this is something that someone else will have to handle.

In general, if anyone has any additional comments/guidelines on how this should be tackled, please do let me know.

@samford samford changed the title Add --locked flag to Rust formulae with a Cargo.lock file Add --locked flag to Rust formulae with a Cargo.lock file Oct 31, 2019
@fxcoudert
Copy link
Member

fxcoudert commented Nov 1, 2019

When updating these formulae, if there isn't a new version available to bump the version at the same time, should we bump the revision?

No. As the formulas are currently working, we don't want to force the new binaries on all users, so no revision bump.

@samford
Copy link
Member Author

samford commented Nov 1, 2019

Out of curiosity, are new bottles published after a formula modification without a revision bump? That is, if a user newly installs a formula after the --locked flag has been added, will they receive a bottle that was built with the --locked flag present?

Besides the demonstrated issue with broken builds over time, my other concern was that without the --locked flag, the bottle may contain a binary that was built using newer crate versions (compared to what's in the lock file). In that case, the build may succeed without errors and the binary may appear to work properly but it wouldn't be an accurate representation of what was intended for that version release.

I'll respect this decision regardless but I felt I would be remiss not to clarify this further.

@jonchang
Copy link
Contributor

jonchang commented Nov 1, 2019

Out of curiosity, are new bottles published after a formula modification without a revision bump? That is, if a user newly installs a formula after the --locked flag has been added, will they receive a bottle that was built with the --locked flag present?

This is possible; you can tell because the formulae will have rebuild 1 or similar in bottle blocks.

@fxcoudert
Copy link
Member

Bottles are systematically rebuilt as part of testing a pull request. Whether the new bottles are published depends on the maintainer's choice when merging, whether they decided to pull the bottles as well during the merge… we almost always do.

@chenrui333
Copy link
Member

Besides nushell.rb (which currently builds on top of beta rust release) and rust.rb, I think every rust formula which has cargo.lock have been addressed.

cc @samford @fxcoudert

@fxcoudert
Copy link
Member

Thanks to everyone involved!

@lock lock bot added the outdated PR was locked due to age label Jan 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

No branches or pull requests

4 participants