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

Remove locked option in std_cargo_args if CPU.arch is arm64 #10205

Closed
wants to merge 1 commit into from

Conversation

shigemk2
Copy link
Contributor

@shigemk2 shigemk2 commented Jan 2, 2021

There are lots of errors in formulae depending rust on arm64
because homebrew installs older dependent cargo packages.
It is because std_cargo_args uses locked option and Cargo.lock.
Homebrew/homebrew-core#68089

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?(same master branch)
  • Have you successfully run brew typecheck with your changes locally?(same master branch)
  • Have you successfully run brew tests with your changes locally?(same master branch)
  • Have you successfully run brew man locally and committed any changes?(same master branch)

There are lots of errors in formulae depending rust on arm64
because homebrew installs older dependent cargo packages.
It is because std_cargo_args uses locked option and Cargo.lock.
@shigemk2 shigemk2 mentioned this pull request Jan 2, 2021
5 tasks
@carlocab
Copy link
Member

carlocab commented Jan 2, 2021

Doesn't the --locked flag require that cargo build use the dependencies declared in Cargo.lock?

If so, then I don't think this is acceptable, even just for ARM. Taking out the --locked flag means that cargo build can use any dependency it wants, and I don't think we like that in Homebrew. It also makes builds non-reproducible.

The better solution might be to ask upstream to cut new releases with updated Cargo.lock files instead.

@shigemk2
Copy link
Contributor Author

shigemk2 commented Jan 2, 2021

Doesn't the --locked flag require that cargo build use the dependencies declared in Cargo.lock?

Yes. cargo install *std_cargo_args will use Cargo.lock because using locked flag.
But will Homebrew require all formulae depending rust (about 120 formulae) to update Cargo.lock if this PR is not accepted?

@carlocab
Copy link
Member

carlocab commented Jan 2, 2021

Do we know that all of them fail due to stale Cargo.lock files? If these files are stale, they should be cutting new releases anyway. Happy to help with filing issues where we need to, though.

That said, I'm open to hearing others' opinions on this.

@samford
Copy link
Member

samford commented Jan 2, 2021

If these files are stale, they should be cutting new releases anyway.

This is my view as well. If the build is failing on ARM due to older dependencies, then affected users should create an issue upstream (making them aware).

As @carlocab mentioned, the --locked flag ensures our builds are reproducible and this is desirable within the context of Homebrew. If the build fails because of Cargo dependencies, it's upstream's responsibility to fix it (though we can always create issues/PRs to help them along).

For what it's worth, I pushed to add the --locked flag to Rust formulae (Homebrew/homebrew-core#46025) in the past and I feel the reasoning there still applies.

@shigemk2
Copy link
Contributor Author

shigemk2 commented Jan 2, 2021

Do we know that all of them fail due to stale Cargo.lock files?

No. I confirmed some errors in formulae was because of Cargo.lock but not all.
The affected areas of bumping Rust are too big, but builds should be reproducible , so we have to create Issues/PRs divide by hands.

@carlocab
Copy link
Member

carlocab commented Jan 2, 2021

we have to create Issues/PRs divide by hands

Yes, I agree that we need an intelligent way of dividing the work on this. I think we could try to enlist user help by opening an issue asking for help to check if cargo build works on a Rust-based formula without the --locked flag but doesn't work with it. If this is the case, then an upstream issue should be filed asking for a new release.

Not sure how to give those users a version of Rust to test with other than asking them to compile the version of the formula in your PR, which would be a pretty big barrier to getting help. Will the above idea work if someone installed the appropriate version of the Rust compiler using rustup instead?

@carlocab
Copy link
Member

carlocab commented Jan 2, 2021

Alternatively, we could script replacing std_cargo_args in all Rust-based formulae to something that leaves out the --locked flag on ARM. Then, we compare the CI run from this to the one before this change and see which formulae have been fixed by dropping the --locked flag.

I think this should identify the formulae with stale lock files. We can then use that information to open the appropriate upstream issues. (We could devise some other procedure for splitting the work on that one.)

@chenrui333
Copy link
Member

Alternatively, we could script replacing std_cargo_args in all Rust-based formulae to something that leaves out the --locked flag on ARM

yeah, we can do that as workaround or submit patches to the upstream.

@samford
Copy link
Member

samford commented Jan 2, 2021

Alternatively, we could script replacing std_cargo_args in all Rust-based formulae to something that leaves out the --locked flag on ARM

yeah, we can do that as workaround or submit patches to the upstream.

Just to be clear, I believe this suggestion was only intended as a way of identifying formulae that fail to build due to the dependencies in the Cargo.lock file. I don't believe @carlocab is suggesting that we disable the --locked flag as a workaround for formulae with related build issues on ARM.

@carlocab
Copy link
Member

carlocab commented Jan 2, 2021

Oh, I don't mean it as a work-around, since I don't think formulae that need the --locked flag dropped should be merged. I just intended it as a way of identifying formulae with stale lock files.

@chenrui333
Copy link
Member

Alternatively, we could script replacing std_cargo_args in all Rust-based formulae to something that leaves out the --locked flag on ARM

yeah, we can do that as workaround or submit patches to the upstream.

Just to be clear, I believe this suggestion was only intended as a way of identifying formulae that fail to build due to the dependencies in the Cargo.lock file. I don't believe @carlocab is suggesting that we disable the --locked flag as a workaround for formulae with related build issues on ARM.

yeah, I am the same page of not disabling --locked, in fact, I did insist to include --locked flag into the rust CI action a while ago.

@fxcoudert
Copy link
Member

We definitely want to keep dependencies pinned, for rust like for everything else in homebrew. Reproducible builds are important.

@fxcoudert fxcoudert closed this Jan 3, 2021
@shigemk2
Copy link
Contributor Author

shigemk2 commented Jan 3, 2021

Thank you for your replying.
I will fix scripts replacing std_cargo_args in all Rust-based formulae.
#10205 (comment)

@fxcoudert
Copy link
Member

@shigemk2 the actual solution is for these formulas to ship a new release with up-to-date dependencies. If we remove the lock files, we might be shipping things that aren't working, or will break, and we have no control. Each upstream should determine what they want as dependencies, and if those aren't compatible with Apple Silicon, then it's simply not supported on Apple Silicon.

@carlocab
Copy link
Member

carlocab commented Jan 3, 2021

@fxcoudert don't worry, that's just a temporary change in order to identify the rust-based formulae with stale lock files.

See #10205 (comment)

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 3, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 3, 2021
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

Successfully merging this pull request may close these issues.

7 participants