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

bootstrap: download-ci-llvm = "if-available" tries to download alt artifacts that don't exist on tier 2 platforms #107225

Closed
Rattenkrieg opened this issue Jan 23, 2023 · 10 comments · Fixed by #107234
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@Rattenkrieg
Copy link
Contributor

Since there is no -alt variation of CI action for dist-aarch64-apple, prebuilt llvm is never published for arm macs.
Hence compete bootstrap like > ./x.py build library will always fail on arm macs with

Copying stage0 library from stage0 (aarch64-apple-darwin -> aarch64-apple-darwin / aarch64-apple-darwin)
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/<commit-hash>/rust-dev-nightly-aarch64-apple-darwin.tar.xz
curl: (22) The requested URL returned error: 404
error: failed to download llvm from ci

help: old builds get deleted after a certain time
help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml:

[llvm]
download-ci-llvm = false

Although it's straightforward to fix by following the hint from error message and finding correct config.codegen.toml (among other config.toml's), bootstrap experience could be better:

  1. Config::download_ci_llvm() could point to correct config file, since there are really few of them;
  2. Config::llvm_from_ci could be set to false for those host triplets that have no published llvm;
  3. Prebuilt llvm could be published for dist-aarch64-apple ie creating dist-aarch64-apple-alt CI action;
  4. Notion of fallback distros could be introduced ie rust-dev-nightly-x86_64-apple-darwin.tar.xz is fallback for rust-dev-nightly-aarch64-apple-darwin.tar.xz.
@Rattenkrieg Rattenkrieg added the C-bug Category: This is a bug. label Jan 23, 2023
@albertlarsan68
Copy link
Member

@rustbot label +T-bootstrap +T-infra

  1. it does point to the correct file: config.toml in the root of your checkout, this is where you should put the config, and not in the src/bootstrap/defaults/config.<insert profile here>.toml
  2. The build should not fail if CI LLVM is not found, it should be built from source instead (is this the real bug?)
  3. cc @rust-lang/infra is this something that is possible / wanted, seeing the proportion of ARM Macs increasing? Should it also be done for the linux build?
  4. I do not think this is really possible, as each target for which rustc is build is unique enough that they are incompatible with each other.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 23, 2023
@Rattenkrieg
Copy link
Contributor Author

@albertlarsan68 thank you for response! All clear with 1. 3. 4.
Regarding 2: I don't think this is a bug in implementation since the logic is explicitly programmed to stop build (exit the process) on missing remote resource. However this is indeed an unpleasant behavior. So as a user, epecialy first time one, I would prefer process to proceed with llvm build, or depending on configuration, fallback to probing local llvm installation.

@albertlarsan68
Copy link
Member

albertlarsan68 commented Jan 23, 2023

I meant that the default is if-available, thus should not fail the build if it can't download CI LLVM, but build it from source as if the config was set to false, and print a warning.

@jyn514
Copy link
Member

jyn514 commented Jan 23, 2023

Prebuilt llvm could be published for dist-aarch64-apple ie creating dist-aarch64-apple-alt CI action;

T-infra said in the meeting today that it's intentional that we don't distribute alt artifacts for tier 2 platforms: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/meeting.202022-01-23/near/323042755. @albertlarsan68 is going to confirm today that this is limited to alt artifacts.

The build should not fail if CI LLVM is not found, it should be built from source instead (is this the real bug?)

👍 this seems useful as long as the error is a 404, but we shouldn't build from source if static.rust-lang.org is having trouble (people should have to explicitly opt-in).

Separately, we may also want to distinguish tier 1 from tier 2 platforms in bootstrap, and not attempt to download the alt artifacts in the first place for tier 2 platforms.

Notion of fallback distros could be introduced ie rust-dev-nightly-x86_64-apple-darwin.tar.xz is fallback for rust-dev-nightly-aarch64-apple-darwin.tar.xz.

I don't like this idea, we shouldn't silently change the platform we're building (and mixing x86 with aarch64 won't work anyway I don't think). For this specific case we could suggest setting llvm-assertions = false or build = "x86_64-apple-darwin" explicitly in config.toml, but it shouldn't be a silent fallback.

@jyn514 jyn514 changed the title [BOOTSTRAP] Unable to download stage0 for aarch64-apple-darwin bootstrap: download-ci-llvm = "if-available" tries to download alt artifacts that don't exist on tier 2 platforms Jan 23, 2023
@Rattenkrieg
Copy link
Contributor Author

I meant that the default is if-available, thus should not fail the build if it can't download CI LLVM, but build it from source as if the config was set to false, and print a warning.

Can confirm, that download attempt and following failure happening when either [llvm] section does not have download-ci-llvm entry or download-ci-llvm is set to "if-available".

Thank you for clarification @jyn514

Seems we may want to remove "aarch64-apple-darwin" from the list of supported platforms. Also bear in mind, that only -alt link leads to 404, for instance https://ci-artifacts.rust-lang.org/rustc-builds/5e37043d63bfe2f3be8fa5a05b07d6c0dad5775d/rust-dev-nightly-aarch64-apple-darwin.tar.xz works fine. So I'm not sure how it fit the logic of supported_platforms list referred above.

@jyn514
Copy link
Member

jyn514 commented Jan 23, 2023

Seems we may want to remove "aarch64-apple-darwin" from the list of supported platforms

This isn't the right fix because it will regress the experience for people using llvm without assertions. See my message above for what I think we should do.

@Rattenkrieg
Copy link
Contributor Author

Seems we may want to remove "aarch64-apple-darwin" from the list of supported platforms

This isn't the right fix because it will regress the experience for people using llvm without assertions. See my message above for what I think we should do.

Ah, I believe I'm getting it. The problem is that supported_platforms has no idea whether we consulting it with downloading -alt distribution in mind or a basic one.

@Rattenkrieg
Copy link
Contributor Author

Patching is_ci_llvm_available() like that

    if triple == "aarch64-apple-darwin" && asserts {
        // No alt builder for aarch64-apple-darwin today.
        return false;
    }

did the trick for me.

@jyn514
Copy link
Member

jyn514 commented Jan 23, 2023

@Rattenkrieg are you interested in making a patch like that for all the tier 2 targets? :) you can find a list here: https://doc.rust-lang.org/nightly/rustc/platform-support.html

(it will probably be easier to have a separate tier 1 and tier 2 list so you don't have to duplicate the names)

@Rattenkrieg
Copy link
Contributor Author

@jyn514 sure, let me try.
Another option would be to have supported_platforms as an array of (&str, bool). So we can do the check like

    if !supported_platforms.contains(&(&*config.build.triple, asserts)) {
        if asserts == true || !supported_platforms.contains(&(&*config.build.triple, true)) {
            return false;
        }
    }

Not sure which one is cleaner/easier to maintain, I'm going to experiment with both.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jan 27, 2023
…vm_available, r=albertlarsan68

Revisit fix_is_ci_llvm_available logic

Fixes rust-lang#107225
Now `supported_platforms` has a knowledge whether llvm asserts artifacts are available for particular host triple.

`@jyn514` `@albertlarsan68` PTAL
@bors bors closed this as completed in 04dfde4 Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants