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

Change the global default for download-ci-llvm to if-available #566

Closed
1 of 3 tasks
jyn514 opened this issue Nov 5, 2022 · 3 comments
Closed
1 of 3 tasks

Change the global default for download-ci-llvm to if-available #566

jyn514 opened this issue Nov 5, 2022 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@jyn514
Copy link
Member

jyn514 commented Nov 5, 2022

Proposal

Switch the None branch of this match to return is_ci_llvm_available instead of false: https:/rust-lang/rust/blob/b0f3940c35d565399dccf8c79f38147b40f2724a/src/bootstrap/config.rs#L1086-L1092

Background and Motivation

x.py has a tiered defaults system. It has a global default, which is used if no config.toml exists, and then opt-in "profiles" (library, compiler, user). Right now, the global default for download_ci_llvm is false and the profile default is "if-available" for all profiles except user. This works generally well for frequent contributors to the compiler, but means that first-time contributors get a sub-standard experience (see for example @pnkfelix's stream where more than half of the first-time build is spent building llvm: https://www.youtube.com/watch?v=oG-JshUmkuA). This is particularly bad because LLVM takes enormously long to build, and it's worse on computers with fewer hardware threads.

I suggest changing the global default to if-available. "if-available" currently means "any tier 1 platform" (there are some additional checks related to running in CI, but they're not important for local development).

Drawbacks

There are two possible downsides:

  • Some platforms appear to be building the LLVM artifacts wrong, and haven't been fixed because not many people use those platforms to build rustc: rust-dev LLVM artifacts are corrupt on FreeBSD 13 rust#96633. We can avoid regressing behavior there by removing them from the if-available check if we notice they're broken.
  • Distros building rustc from source will not like this new behavior and will have to opt-out with ./configure --set llvm.download-ci-llvm=false. I think this is fine as long as the change is documented in bootstrap's changelog.

Mentors or Reviewers

@pnkfelix for helping me convince t-compiler this is a good idea
@Mark-Simulacrum for thinking of any downsides I may have missed

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@jyn514 jyn514 added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Nov 5, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Nov 5, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Nov 5, 2022

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Nov 5, 2022
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Nov 16, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 18, 2022
jyn514 added a commit to jyn514/rust that referenced this issue Nov 20, 2022
…ev"`

See rust-lang/compiler-team#566.
The motivation for changing the default is to avoid downloading and building LLVM when someone runs `x build` before running `x setup`.
The motivation for only doing it on `channel = "dev"` is to avoid breaking distros or users installing from source. It works because `dev` is also the default channel.

The diff looks larger than it is; most of it is moving the `llvm` branch below the `rust` so `config.channel` is set.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 10, 2022
…=Mark-Simulacrum

Set `download-ci-llvm = "if-available"` by default when `channel = dev`

See rust-lang/compiler-team#566. The motivation for changing the default is to avoid downloading and building LLVM when someone runs `x build` before running `x setup`. The motivation for only doing it on `channel = "dev"` is to avoid breaking distros or users installing from source. It works because `dev` is also the default channel.

The diff looks larger than it is; most of it is moving the `llvm` branch below the `rust` so `config.channel` is set.

r? `@Mark-Simulacrum` cc `@oli-obk` `@bjorn3` `@cuviper`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants