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

issue #45357 don't build clippy stage 1 #45548

Merged
merged 2 commits into from
Nov 4, 2017
Merged

Conversation

ratmice
Copy link
Contributor

@ratmice ratmice commented Oct 26, 2017

#45357
Wasn't sure top_stage was the right thing, but seemed to go ahead building clippy stage 2.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2017
@kennytm
Copy link
Member

kennytm commented Oct 26, 2017

r? @Mark-Simulacrum

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2017

There are other crates mentioned in the issue. Can you limit them to stage 2, too?

@ratmice
Copy link
Contributor Author

ratmice commented Oct 28, 2017

@oli-obk Good catch, I didn't run into any issues with rustdoc or cargo,
looking a bit closer perhaps these are already handled, I will try using the same thing as cargo's default_condition, however my turn around time on this is fairly slow.

For rustdoc I believe it is handled here:
Build rustdoc only at the top stage

With Cargo I think perhaps it is handled by this line:
Add an optional condition to constrain defaults.

Where the should_run for rustdoc has no such condition.
rustdoc should_run

Edit: Retrying with extended=true in config.toml to see if that won't induce a failing cargo build.

@ratmice
Copy link
Contributor Author

ratmice commented Oct 29, 2017

@oli-obk So .default_condition(builder.build.config.extended) does appear to limit building cargo (and clippy when set) to stage 2 when extented = true in config.toml, However I don't see where this happens occur.

Anyhow, I'm at a bit of a loss as to whether clippy needs the condition set instead of my patch,
Or cargo needs something like the match expression from my patch added.
Some of that depends upon whether we really want clippy default, or just in the set of extended tools.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2017

The extended check for clippy makes sense. A stage 1 build without any flags should not build clippy even if it could be built on stage 1

@ratmice
Copy link
Contributor Author

ratmice commented Oct 29, 2017

@oli-obk I updated the branch to set the extended condition instead of the match.
If it gets merged, by all means squash it. I imagine further cleanups (if any) to make the relationship between extended/stage more apparent could possibly be done separately?

@shepmaster
Copy link
Member

@oli-obk do you want to take over review of this? Or should @Mark-Simulacrum still review?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 3, 2017

Sure. Who do I ping for the final r+? Or does the bors-delegate thing work?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 3, 2017

@ratmice I think you forgot to push your changes ;)

@kennytm
Copy link
Member

kennytm commented Nov 3, 2017

Let's try...

@bors delegate=oli-obk

@bors
Copy link
Contributor

bors commented Nov 3, 2017

✌️ @oli-obk can now approve this pull request

@ratmice
Copy link
Contributor Author

ratmice commented Nov 3, 2017

@oli-obk whoops, pushed a branch on accident, should be fixed now.

@@ -415,7 +415,7 @@ impl Step for Clippy {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/tools/clippy")
run.path("src/tools/clippy").default_condition(run.builder.build.config.extended)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compilation error. We don't have NLL in stage0 yet...

[00:01:24]    Compiling bootstrap v0.0.0 (file:///checkout/src/bootstrap)
[00:01:28] error[E0382]: use of moved value: `run.builder.build.config.extended`
[00:01:28]    --> /checkout/src/bootstrap/tool.rs:418:55
[00:01:28]     |
[00:01:28] 418 |        run.path("src/tools/clippy").default_condition(run.builder.build.config.extended)
[00:01:28]     |        --- value moved here                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value used here after move
[00:01:28]     |
[00:01:28]     = note: move occurs because `run` has type `builder::ShouldRun<'_>`, which does not implement the `Copy` trait

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also that one missing space should be re-inserted ;)

@ratmice
Copy link
Contributor Author

ratmice commented Nov 3, 2017

@kennytm pushed the wrong patch to the right branch this time, apparently you can create an ambiguous branch in git, and it makes things confusing. Apologies.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 3, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2017

📌 Commit aa9d0aa has been approved by oli-obk

kennytm added a commit to kennytm/rust that referenced this pull request Nov 4, 2017
issue rust-lang#45357 don't build clippy stage 1

rust-lang#45357
Wasn't sure top_stage was the right thing, but seemed to go ahead building clippy stage 2.
bors added a commit that referenced this pull request Nov 4, 2017
Rollup of 9 pull requests

- Successful merges: #45548, #45610, #45639, #45669, #45681, #45718, #45722, #45739, #45746
- Failed merges:
@bors
Copy link
Contributor

bors commented Nov 4, 2017

⌛ Testing commit aa9d0aa with merge 9acc333...

@bors bors merged commit aa9d0aa into rust-lang:master Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants