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

Verify that the tools are actually distributed on beta/stable channels #54483

Closed
kennytm opened this issue Sep 22, 2018 · 7 comments · Fixed by #54638
Closed

Verify that the tools are actually distributed on beta/stable channels #54483

kennytm opened this issue Sep 22, 2018 · 7 comments · Fixed by #54638
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@kennytm
Copy link
Member

kennytm commented Sep 22, 2018

In #54206 it was noted that RLS was missing in x86_64-pc-windows-gnu, but toolstate didn't see the issue because it only checks x86_64-pc-windows-msvc and x86_64-unknown-linux-gnu. We ought to ensure that the tools are not forgotten at least on all tier-1 platforms.

The simplest fix right now is to add a check that if a tool failed to build, the whole dist job should be aborted. Currently we allowed tool dist to fail because of this:

rust/src/bootstrap/dist.rs

Lines 1166 to 1169 in e7b5ba8

let rls = builder.ensure(tool::Rls {
compiler: builder.compiler(stage, builder.config.build),
target, extra_features: Vec::new()
}).or_else(|| { println!("Unable to build RLS, skipping dist"); None })?;

similar for Clippy:

rust/src/bootstrap/dist.rs

Lines 1245 to 1252 in e7b5ba8

let clippy = builder.ensure(tool::Clippy {
compiler: builder.compiler(stage, builder.config.build),
target, extra_features: Vec::new()
}).or_else(|| { println!("Unable to build clippy, skipping dist"); None })?;
let cargoclippy = builder.ensure(tool::CargoClippy {
compiler: builder.compiler(stage, builder.config.build),
target, extra_features: Vec::new()
}).or_else(|| { println!("Unable to build cargo clippy, skipping dist"); None })?;

and rustfmt:

rust/src/bootstrap/dist.rs

Lines 1323 to 1331 in e7b5ba8

// Prepare the image directory
let rustfmt = builder.ensure(tool::Rustfmt {
compiler: builder.compiler(stage, builder.config.build),
target, extra_features: Vec::new()
}).or_else(|| { println!("Unable to build Rustfmt, skipping dist"); None })?;
let cargofmt = builder.ensure(tool::Cargofmt {
compiler: builder.compiler(stage, builder.config.build),
target, extra_features: Vec::new()
}).or_else(|| { println!("Unable to build Cargofmt, skipping dist"); None })?;

What we should do is modify the or_else branch to panic! unless we are configured to allow omitting these tools (i.e. in the nightly channel). (Consider refactor this into the same function to avoid writing the same thing 5 times.)

While it is possible to check the release channel directly inside rustbuild, I consider this a bad coupling. It is better to add a configuration variable --enable-missing-tools, and modify src/ci/run.sh to pass this into $RUST_CONFIGURE_ARGS when

  1. $RUST_RELEASE_CHANNEL = "nightly", or
  2. the env var $DIST_REQUIRE_ALL_TOOLS is undefined

You may see #39824 for how to add a config to rustbuild.

After this issue is fixed, we'll ensure RLS/Clippy/Rustfmt will be present on all tier-1 platforms on beta/stable. In the future we should record and alert the tool developers when they fail to build on nightly or lower-tier platforms, but let's get the immediate issue fixed first...

@kennytm kennytm added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Sep 22, 2018
@pvdrz
Copy link
Contributor

pvdrz commented Sep 23, 2018

HI @kennytm! I'd like to take care of this one

@kennytm
Copy link
Member Author

kennytm commented Sep 24, 2018

@christianpoveda Thanks!

@pvdrz
Copy link
Contributor

pvdrz commented Sep 28, 2018

I started working on this here https://github.com/christianpoveda/rust/commit/2d93ce6cfdcce9d538644d7e607777d32c33a09d. However, I'm not sure if everything is where it should.

@kennytm
Copy link
Member Author

kennytm commented Sep 28, 2018

@christianpoveda Looks fine (except all the strange whitespace changes on src/bootstrap/configure.py). Next you'll need to

  1. add ENV DIST_REQUIRE_ALL_TOOLS 1 to the tier-1 Linux Dockerfiles in src/ci/docker/dist-*/
  2. add DIST_REQUIRE_ALL_TOOLS=1 to the tier-1 macOS dist jobs in .travis.yml
  3. add DIST_REQUIRE_ALL_TOOLS: 1 to the tier-1 Windows dist jobs in appveyor.yml

@pvdrz
Copy link
Contributor

pvdrz commented Sep 28, 2018

Ok I'll do the changes to the dockerfiles and CI configuration files.

PD: the strange whitespaces are YAPF's fault. I'll fix them

@pvdrz
Copy link
Contributor

pvdrz commented Sep 28, 2018

@kennytm
Copy link
Member Author

kennytm commented Sep 28, 2018

We don't really need DIST_REQUIRE_ALL_TOOLS: 1 for the -alt builds. Anyway, you could prepare a PR now and see how the bots react.

bors added a commit that referenced this issue Sep 28, 2018
Add checking for tool distribution in Tier 1

This fixes #54483
r? @kennytm
kennytm added a commit to kennytm/rust that referenced this issue Oct 3, 2018
Add checking for tool distribution in Tier 1

This fixes rust-lang#54483
r? @kennytm
bors added a commit that referenced this issue Oct 4, 2018
Add checking for tool distribution in Tier 1

This fixes #54483
r? @kennytm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. 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.

2 participants