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

Prevent building the clippy plugin in rustbuild #66052

Closed
wants to merge 1 commit into from

Conversation

msizanoen1
Copy link
Contributor

@msizanoen1 msizanoen1 commented Nov 3, 2019

Provided the tool names as defined using tool_extended!(...) are valid binary names as defined in the manifest, this PR avoids building the clippy plugin by passing the --bin flags followed by tool name to Cargo when building extended tools.
If we don't pass the --bin flag, Cargo will build everything by default - including the plugin that causes the failure.
Close #62558

@msizanoen1

This comment has been minimized.

@mati865
Copy link
Contributor

mati865 commented Nov 3, 2019

I cannot approve PRs, you need to r? on proper reviewer.

@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Nov 3, 2019

r? @alexcrichton

@msizanoen1
Copy link
Contributor Author

Some local testing shows that clippy builds sucessfully after this change.

@Mark-Simulacrum
Copy link
Member

Can you confirm that all the tools are named the same as the tool? I am not sure that's the case, if it isn't we'll likely want to change the logic here a bit.

@JohnTitor JohnTitor added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 3, 2019
@alexcrichton
Copy link
Member

Thanks for the PR! I'm sort of confused though as to why this fixes that issue, could you expand in the PR description, the comments, or the commit message about how this fixes the original issue?

@msizanoen1
Copy link
Contributor Author

I have updated the description.

@alexcrichton
Copy link
Member

Could you also place some comments in the source code about how tools are being avoided?

Additionally does this affect rustfmt as well? There I believe we want both the rustfmt and cargo fmt executables.

Could we otherwise update clippy to put the plugin behind an off-by-default feature or move it to a separate crate?

@msizanoen1
Copy link
Contributor Author

I added a comment.
Also rustfmt should work fine: https://github.com/rust-lang/rustfmt/blob/master/Cargo.toml

@alexcrichton
Copy link
Member

Can you expand on your reasoning on why rustfmt will still work? Simply pasting a link to the manifest is pretty opaque and doesn't really mean much to me. It doesn't answer my concern about this probably not building the cargo-fmt executable since it'd only build rustfmt

@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Nov 5, 2019

The tools declaration is here:

rust/src/bootstrap/tool.rs

Lines 642 to 659 in d1fff4a

tool_extended!((self, builder),
Cargofmt, rustfmt, "src/tools/rustfmt", "cargo-fmt", {};
CargoClippy, clippy, "src/tools/clippy", "cargo-clippy", {};
Clippy, clippy, "src/tools/clippy", "clippy-driver", {};
Miri, miri, "src/tools/miri", "miri", {};
CargoMiri, miri, "src/tools/miri", "cargo-miri", {};
Rls, rls, "src/tools/rls", "rls", {
let clippy = builder.ensure(Clippy {
compiler: self.compiler,
target: self.target,
extra_features: Vec::new(),
});
if clippy.is_some() {
self.extra_features.push("clippy".to_owned());
}
};
Rustfmt, rustfmt, "src/tools/rustfmt", "rustfmt", {};
);

As you can see, the cargo-fmt and rustfmt binaries are defined with the same name both in this file and the manifest:
https://github.com/rust-lang/rustfmt/blob/6aec17e4c372d1cda3484a2e292cff8f5995dd64/Cargo.toml#L14-L20

@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Nov 5, 2019

Also I have tested this locally with a full extended build.

@mati865
Copy link
Contributor

mati865 commented Nov 5, 2019

Clippy should drop plugin interface as soon as it's CI is fixed: rust-lang/rust-clippy#4714
Is it worth to make additional effort here?

@msizanoen1
Copy link
Contributor Author

Closed in favor of #66142

@msizanoen1 msizanoen1 closed this Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tools are not available on cross compiled platforms since #61861
5 participants