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

Add clippy to toolstate.toml #44679

Merged
merged 2 commits into from
Sep 21, 2017
Merged

Add clippy to toolstate.toml #44679

merged 2 commits into from
Sep 21, 2017

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 18, 2017

r? @alexcrichton

cc @Manishearth

I have no idea how to get clippy working... it needs proc macros, and I think I did everything right (I just did what the cargo step is doing), but it's not working:

error: libproc_macro-6210e4b46662ec28.so: cannot open shared object file: No such file or directory
  --> src/tools/clippy/clippy_lints/src/lib.rs:47:1
   |
47 | extern crate serde_derive;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: libproc_macro-6210e4b46662ec28.so: cannot open shared object file: No such file or directory
  --> src/tools/clippy/clippy_lints/src/lib.rs:47:1
   |
47 | extern crate serde_derive;
   | ^

It's especially weird since it used to work

Anyway. Fixing it can be left for a future PR, this one adds it to CI, but marks it as "broken"

@alexcrichton
Copy link
Member

cc @rust-lang/dev-tools, any thoughts about this? The tl;dr; here is that you should be able to just change a line in src/tools/toolstate.toml to say "don't build clippy on this PR", so that way it's in theory easy for contributors to not have to go through the whole submodule process to land a PR


// Don't build tests dynamically, just a pain to work with
cargo.env("RUSTC_NO_PREFER_DYNAMIC", "1");
// miri tests need to know about the stage sysroot
Copy link
Member

Choose a reason for hiding this comment

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

s/miri/clippy?

do we need to still know the sysroot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Manishearth
Copy link
Member

I'm +1 on this. We should probably encourage that folks try to get a working patch to fix it at least (even if they then hand it off to someone else to coordinate landing). But the general tooling sounds good and easy to use.

@nrc
Copy link
Member

nrc commented Sep 18, 2017

I'm +1 on adding Clippy to toolstate.toml. I don't think we should enable testing by default at this stage - I'm not confident enough of the workflow here just yet (I'm in favour of taking this super-slowly because I worry about causing issues for compiler devs or of tools getting broken and disabled but not fixed, and I think it is worth getting the workflow right for all tools, rather than rushing to push Clippy forward).

@Manishearth
Copy link
Member

(Since this confused me, nrc clarified that he's only referring to postponing testing, not building clippy.)

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 19, 2017

I would prefer to allow changing the toolstate to testing, if only so we get pinged when it happens and the state is downgraded to building. It is not possible to change the toolstate to broken if it is still building. So there won't be any "overreaction" on the Dev side.

That said, I can make the testing pass not run by default, which will mean that even if the state is changed to testing, it'll require explicitly running ./x.py test src/tools/clippy in order for the testing state to act differently from the compiling state

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 19, 2017

I disabled clippy testing without passing a full path

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 19, 2017

  • Travis likes it now
  • clippy is set to "Broken" until the proc-macro thing has been fixed (any help is appreciated)
  • setting clippy to "Testing" is just the same as setting it to "Building" except when explicitly calling ./x.py test src/tools/clippy

@alexcrichton
Copy link
Member

Just to ensure that we're continuing to take this slowly, can this continue to disable clippy by default?

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 19, 2017

Done

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 19, 2017

📌 Commit 7d7e7d4 has been approved by alexcrichton

@arielb1 arielb1 added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 19, 2017
@bors
Copy link
Contributor

bors commented Sep 21, 2017

⌛ Testing commit 7d7e7d4 with merge 1b55d19...

bors added a commit that referenced this pull request Sep 21, 2017
Add clippy to `toolstate.toml`

r? @alexcrichton

cc @Manishearth

I have no idea how to get clippy working... it needs proc macros, and I think I did everything right (I just did what the cargo step is doing), but it's not working:

```
error: libproc_macro-6210e4b46662ec28.so: cannot open shared object file: No such file or directory
  --> src/tools/clippy/clippy_lints/src/lib.rs:47:1
   |
47 | extern crate serde_derive;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: libproc_macro-6210e4b46662ec28.so: cannot open shared object file: No such file or directory
  --> src/tools/clippy/clippy_lints/src/lib.rs:47:1
   |
47 | extern crate serde_derive;
   | ^
```

It's especially weird since it used to work

Anyway. Fixing it can be left for a future PR, this one adds it to CI, but marks it as "broken"
@bors
Copy link
Contributor

bors commented Sep 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1b55d19 to master...

@bors bors merged commit 7d7e7d4 into rust-lang:master Sep 21, 2017
@oli-obk oli-obk deleted the clippy_ci branch June 15, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants