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

Fix Rust compiler download error on Android #17604

Closed
wants to merge 1 commit into from

Conversation

@MortimerGoro
Copy link
Contributor

MortimerGoro commented Jul 5, 2017

After 6b52330 landed Android builds fail when downloading Rustc Compiler:

Downloading Host rust library for target armv7-linux-androideabi...
Download failed (404): Not Found - https://s3.amazonaws.com/rust-lang-ci/rustc-builds-alt/3bfc18a9619a5151ff4f11618db9cd882996ba6f/rust-std-nightly-armv7-linux-androideabi.tar.gz

I asked in rust-infra and they said that alt builds aren't compiled yet for all platforms (e.g. Android)


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Jul 5, 2017

Heads up! This PR modifies the following files:

@nox
Copy link
Member

nox commented Jul 5, 2017

I would rather have us error out if llvm-assertions is set on a platform that doesn't support that. What do you think @jdm?

@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2017

This isn’t specific to Android. We have an explicit list of platforms that have these builds:

# https://github.com/rust-lang/rust/pull/39754
platforms_with_rustc_alt_builds = ["unknown-linux-gnu", "apple-darwin", "pc-windows-msvc"]
llvm_assertions_default = ("SERVO_RUSTC_LLVM_ASSERTIONS" in os.environ
or host_platform() not in platforms_with_rustc_alt_builds)

It looks like the problem is that that code only checks the host platform, not the target when cross-compiling. I wonder how #17561 landed though, since we do have CI for Android. Anyway, this should use the same list of platforms (maybe by making it a Python global variable).

@MortimerGoro MortimerGoro force-pushed the MortimerGoro:android_alt_builds branch 2 times, most recently from 3a49129 to 996e4ca Jul 5, 2017
@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Jul 5, 2017

Ok, I updated the PR to also check target platform in order to set llvm-assertions default configuration value

@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2017

Does this really fix the issue? The code added in bootstrap_rustc is executed well after CommandBase.__init__, modifying self.config this late often doesn’t work (since whatever code reads the config has already executed).

Specifically here, I think the rust_path variable would be wrong.

@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2017

Taking a step back, do we need to use a "non-alt" compiler when using a non-alt standard library (because the alt one is not available for a given cross-compilation target)? If so, I think we have a problem with with how mach’s configuration is currently structured.

@MortimerGoro MortimerGoro force-pushed the MortimerGoro:android_alt_builds branch from 996e4ca to 345b9eb Jul 5, 2017
@MortimerGoro MortimerGoro force-pushed the MortimerGoro:android_alt_builds branch from 345b9eb to 145e2ac Jul 5, 2017
@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Jul 5, 2017

Does this really fix the issue? The code added in bootstrap_rustc is executed well after CommandBase.init, modifying self.config this late often doesn’t work (since whatever code reads the config has already executed).

Specifically here, I think the rust_path variable would be wrong.

I moved the code before using rust_path var. I have tested to compile, link and run on a clean directory and it worked.

Is there a better place for that code? We need to use the target argument.

@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2017

By on a clean directory, do you mean you also removed .servo/rust? If you do that and then compile for Android, is rustc not downloaded into .servo/rust/*-alt despite being a non-alt build? If you clean everything and then build twice (changing a source file in-between to force some re-compilation), does the second build (which should not go through bootstap_rust again) still work?

@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2017

We need to use the target argument.

Yeah, that’s the problem. Command-line arguments are not easily available in Command.__init__.

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Jul 5, 2017

I reproduced the error. On a clean checkout:

  1. ./mach build --release --android builds OK. New non-alt rust compiler in .servo/rust/
  2. code change && ./mach build --release --android builds OK (shows Rust compiler already downloaded message.). The same non-alt rust compiler in .servo/rust/
  3. ./mach build --release builds OK. New alt rustc compiler in .servo/rust/
  4. ./mach build --release --android fails when the alt rustc compiler exists:
error[E0463]: can't find crate for `std`
  |
  = note: the `armv7-linux-androideabi` target may not be installed
@SimonSapin
Copy link
Member

SimonSapin commented Jul 6, 2017

@MortimerGoro Until we figure something out, you can copy servobuild.example to .servobuild and in the [build] section replace #llvm-assertions = false with llvm-assertions = true. This will force alternate compilers to never be used, so you shouldn’t have any bootstrapping problem.

SimonSapin added a commit that referenced this pull request Jul 14, 2017
This reverts commit 6b52330.

This is unnecessary now that rust-lang/rust#42967
is fixed by rust-lang/rust#43167.

This migth be a fix for #17604
bors-servo added a commit that referenced this pull request Jul 16, 2017
Upgrade to rustc 1.20.0-nightly (ab91c70cc 2017-07-14), use non-"alt" std

Possibly fix #17604

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17727)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jul 17, 2017
Upgrade to rustc 1.20.0-nightly (ab91c70cc 2017-07-14), use non-"alt" std

<s>Possibly</s> fixes #17604

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17727)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 18, 2017
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : a3b91220837ef27717c62df0b8ff56a4e94b9b87
weilonge pushed a commit to weilonge/gecko that referenced this pull request Jul 18, 2017
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Jul 18, 2017
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request Jul 18, 2017
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096
leobalter pushed a commit to leobalter/gecko-dev that referenced this pull request Jul 20, 2017
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Jul 24, 2017
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096

UltraBlame original commit: 8321474ca566ca5c3a7b137b16baccb97d9efe5a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096

UltraBlame original commit: 8321474ca566ca5c3a7b137b16baccb97d9efe5a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…-07-14), use non-"alt" std (from servo:rustup); r=nox

<s>Possibly</s> fixes servo/servo#17604

Source-Repo: https://github.com/servo/servo
Source-Revision: 9d30e5b4e0fe9ccdfcd47d5134b5fbfba2e68096

UltraBlame original commit: 8321474ca566ca5c3a7b137b16baccb97d9efe5a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.