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

rustup-init: install proxy binaries #177582

Merged
merged 2 commits into from
Jul 19, 2024
Merged

Conversation

branchvincent
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Currently, this formula only contains a rustup-init binary (an installer) which a user must call once (and then will never need it again). It's not really useful in its current form because users can just do curl https://sh.rustup.rs | sh to achieve the same.

Instead, this actually installs all rustup-managed proxies (which are all just rustup symlinked under a different name). All users need to do is configure a default toolchain on the first run.

The only downside is conflicting with rust, which was noted in the original PR #9617 but I think we should reconsider since it gives users the choice (and most users won't notice now that we have bottles and rust is only a build dep). It is also the same approach other packagers seem to have taken: arch, debian, nix

@github-actions github-actions bot added the rust Rust use is a significant feature of the PR or issue label Jul 17, 2024
@github-actions github-actions bot added CI-build-dependents-from-source Pass --build-dependents-from-source to brew test-bot. long build Set a long timeout for formula testing long dependent tests Set a long timeout for dependent testing labels Jul 17, 2024
@branchvincent
Copy link
Member Author

Ah but we need rust to build rustup. Let me rethink this

@branchvincent
Copy link
Member Author

Ok moved proxy binaries to libexec to avoid conflicts

@branchvincent branchvincent marked this pull request as ready for review July 17, 2024 07:06
@branchvincent branchvincent added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Jul 17, 2024
@carlocab
Copy link
Member

The only downside is conflicting with rust, which was noted in the original PR #9617 but I think we should reconsider since it gives users the choice (and most users won't notice now that we have bottles and rust is only a build dep). It is also the same approach other packagers seem to have taken: arch, debian, nix

We could probably make one of them keg-only to fix the conflict.

@branchvincent
Copy link
Member Author

I do like that approach more since users could also brew link --force. Looking at analytics:

rust:

install: 38,827 (30 days), 113,069 (90 days), 440,514 (365 days)
install-on-request: 28,106 (30 days), 81,005 (90 days), 333,059 (365 days)

rustup-init:

install: 3,643 (30 days), 16,870 (90 days), 51,411 (365 days)
install-on-request: 3,393 (30 days), 15,613 (90 days), 47,587 (365 days)

So I say we make rustup-init keg-only, rename it to rustup (note we do have an existing alias) and stop installing the rustup-init binary (since it no longer serves a purpose). Only issue is breaking current installs, do we care? We could of course just note it in the caveats or instead deprecate rustup-init and make rustup a new formulae

@carlocab
Copy link
Member

We could manually link rustup and rustup-init in post_install since those wouldn't conflict, I think.

@branchvincent
Copy link
Member Author

great idea, done! This looks pretty good to me now, my only other nit is I want to rename this to rustup and have rustup-init be the alias instead, but I can save that for another PR

@branchvincent branchvincent added CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. and removed CI-build-dependents-from-source Pass --build-dependents-from-source to brew test-bot. long build Set a long timeout for formula testing long dependent tests Set a long timeout for dependent testing labels Jul 19, 2024
Copy link
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Jul 19, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue Jul 19, 2024
Merged via the queue into Homebrew:master with commit 435f2c4 Jul 19, 2024
15 checks passed
@branchvincent branchvincent deleted the rustup branch July 19, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. rust Rust use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants