📌 Pin Clippy to a nightly 📌 #6401

ebroto · 2020-11-29T17:32:23Z

changelog: Pin Clippy to a specific nightly version (No more master/custom toolchain required to compile Clippy)

Addresses partially #5561. As proposed there in this comment, this kicks off the process, to help us get acquainted with how the syncs should work, before working on improving the tooling.

Open questions:

When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that.
When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful.
Who does the rustups now? If we follow my previous idea and do both rustup and clippyup at the same time, it would be more work for @flip1995 who currently does the clippyups. I would prefer to establish some kind of rotation to spead the work. Other ideas?
I'm not sure if this affects the release process in any way.
???

@rust-lang/clippy thoughts?

r? @flip1995

ebroto · 2020-11-29T17:40:12Z

(converting to draft as I want to test the changes manually in the rustc repo)

matthiaskrgr · 2020-11-29T19:05:09Z

When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released.

Can we specify a commit up to that we sync when pulling from rustc into rust-lang/clippy?
If so, maybe we could have some kind of script which finds out the latest version of rustc nightly (perhaps via rustup check?) and performs a sync up to that rust-nightly commit, and then edits the rust-toolchain file accordingly?

flip1995 · 2020-11-29T20:25:33Z

When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that.

I like @matthiaskrgr idea. So the process of a Rustup would be to check the commit of the latest nightly, then checkout this commit in the Rust repo and do the sync from the detached head.

We can build tooling for this later. There is also gitree, which @mominul implemented and is still waiting on my review. I suggest to enhance this tool and use it.

When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful.

Yeah, we should do it together with (right after) the rustups. Rustup PRs should get p=1 in the rust repo, so that the process is faster. If the process should get painful, we can do those rustups every week, instead of every other week.

Who does the rustups now?

I don't mind doing them, it's really not that much work. I'll post on Zulip if I should not get to do them for some reason.

I'm not sure if this affects the release process in any way.

I don't think so? 🤔 We'll see, I guess

flip1995

I'll have to do in depth review and think about this for a bit longer, so that we don't forget about something. Thanks for moving this forward ❤️

CONTRIBUTING.md

setup-toolchain.sh

matthiaskrgr · 2020-11-29T21:22:44Z

Right now, the rustfmt component is missing from nightly and it seems that when building clippy with a nightly that does not offer a required (in the rust-toolchain file) component, rustup simply skips the component.

info: syncing channel updates for 'nightly-2020-11-29-x86_64-unknown-linux-gnu'
info: latest update on 2020-11-29, rust version 1.50.0-nightly (e37f25aa3 2020-11-28)
warning: Force-skipping unavailable component 'rustfmt-x86_64-unknown-linux-gnu'
info: downloading component 'cargo'
...

This might be something to be aware of, I hope it won't break our neck later at some point....

It also installs clippy from that nightly for some reason which seems a bit excessive? 😄

ebroto · 2020-11-29T21:32:07Z

Right now, the rustfmt component is missing from nightly and it seems that when building clippy with a nightly that does not offer a required (in the rust-toolchain file) component, rustup simply skips the component.

If I'm not mistaken the situation should not be different to what we have now as we are using rustfmt from the nightly (without a specific date) toolchain for the tests.

It also installs clippy from that nightly for some reason which seems a bit excessive? smile

Yeah, that was an oversight when adding TOML support for the toolchain file, it installs the components in the current profile in addition to the ones specified. There's rust-lang/rustup#2579 to address it.

In the CI we set the minimal profile before starting the build, so it does not try to install clippy.

doc/basics.md

ebroto · 2020-11-30T21:33:14Z

After testing this in the in-tree version of Clippy, I think it does not cause additional problems. I ran into the Found multiple rlibs for crate serde error, but I think it's not related to this change.

I've opened a Zulip thread to discuss that problem, but I think it was already present.

Manishearth · 2020-11-30T23:26:48Z

I am a bit iffy about this: we used to pin to nightlies and dealing with the gap period between master merges and nightlies was really annoying. But now that rustups can happen without us needing to fix things, perhaps this just means we need to run syncs at appropriate times. That seems fine.

flip1995 · 2020-12-01T18:27:50Z

But now that rustups can happen without us needing to fix things, perhaps this just means we need to run syncs at appropriate times. That seems fine.

Yeah, now it should just be git sbtree push and git mergetool. We then have to figure out the right cycle, so that the mergetool won't be overwhelmed.

I'll try to review this over the weekend 👍

flip1995

LGTM, let's do this with #6401 (comment) addressed 🚀

Also the sync should be in the order

subtree push to the Clippy repo
subtree pull to the Rust repo

That way we make sure, that both Clippy and Rust pass the complete Clippy test suite after each sync.

Also somewhere should be documented, that you want to sync from Rust to Clippy from the latest nightly commit (rustup check), not the top-of-tree commit.

ebroto · 2020-12-09T16:22:53Z

I've addressed the last remark and updated to the latest nightly to be coherent with the latest rustup we did yesterday.

flip1995 · 2020-12-09T17:28:11Z

@bors r+

Let's go, thanks for moving this forward! 🚀

Next steps:

Adapt .rustfmt.toml from rustc
Try to use the same nightly rustfmt as rustc
Let the tidy check in rustc also check Clippy.

bors · 2020-12-09T17:28:13Z

📌 Commit 945536b has been approved by flip1995

📌 Pin Clippy to a nightly 📌 changelog: none Addresses partially #5561. As proposed there in [this comment](#5561 (comment)), this kicks off the process, to help us get acquainted with how the syncs should work, before working on improving the tooling. Open questions: * When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that. * When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful. * Who does the rustups now? If we follow my previous idea and do both rustup and clippyup at the same time, it would be more work for `@flip1995` who currently does the clippyups. I would prefer to establish some kind of rotation to spead the work. Other ideas? * I'm not sure if this affects the release process in any way. * ??? `@rust-lang/clippy` thoughts? r? `@flip1995`

bors · 2020-12-09T17:29:50Z

⌛ Testing commit 945536b with merge e5bb0ab...

bors · 2020-12-09T17:39:31Z

💔 Test failed - checks-action_test

flip1995 · 2020-12-09T17:53:49Z

Off to a good start 😄

I don't think we need the steps, which set the LD_LIBRARY_PATH and equivalents anymore.

flip1995 · 2020-12-09T18:02:14Z

This patch should do the job:

diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml
index 181b3bab4..7a9a2893c 100644
--- a/.github/workflows/clippy.yml
+++ b/.github/workflows/clippy.yml
@@ -38,7 +38,6 @@ jobs:
     - name: rust-toolchain
       uses: actions-rs/toolchain@v1.0.6
       with:
-        toolchain: nightly
         target: x86_64-unknown-linux-gnu
         profile: minimal

@@ -56,12 +55,6 @@ jobs:
         restore-keys: |
           ${{ runner.os }}-x86_64-unknown-linux-gnu

-    # Run
-    - name: Set LD_LIBRARY_PATH (Linux)
-      run: |
-        SYSROOT=$(rustc --print sysroot)
-        echo "LD_LIBRARY_PATH=${SYSROOT}/lib${LD_LIBRARY_PATH+:${LD_LIBRARY_PATH}}" >> $GITHUB_ENV
-
     - name: Build
       run: cargo build --features deny-warnings

diff --git a/.github/workflows/clippy_bors.yml b/.github/workflows/clippy_bors.yml
index f08182365..06fb314d1 100644
--- a/.github/workflows/clippy_bors.yml
+++ b/.github/workflows/clippy_bors.yml
@@ -87,7 +87,6 @@ jobs:
     - name: rust-toolchain
       uses: actions-rs/toolchain@v1.0.6
       with:
-        toolchain: nightly
         target: ${{ matrix.host }}
         profile: minimal

@@ -106,23 +105,6 @@ jobs:
           ${{ runner.os }}-${{ matrix.host }}

     # Run
-    - name: Set LD_LIBRARY_PATH (Linux)
-      if: runner.os == 'Linux'
-      run: |
-        SYSROOT=$(rustc --print sysroot)
-        echo "LD_LIBRARY_PATH=${SYSROOT}/lib${LD_LIBRARY_PATH+:${LD_LIBRARY_PATH}}" >> $GITHUB_ENV
-    - name: Link rustc dylib (MacOS)
-      if: runner.os == 'macOS'
-      run: |
-        SYSROOT=$(rustc --print sysroot)
-        sudo mkdir -p /usr/local/lib
-        sudo find "${SYSROOT}/lib" -maxdepth 1 -name '*dylib' -exec ln -s {} /usr/local/lib \;
-    - name: Set PATH (Windows)
-      if: runner.os == 'Windows'
-      run: |
-        SYSROOT=$(rustc --print sysroot)
-        echo "$SYSROOT/bin" >> $GITHUB_PATH
-
     - name: Build with internal lints
       run: cargo build --features deny-warnings,internal-lints

@@ -169,7 +151,6 @@ jobs:
     - name: rust-toolchain
       uses: actions-rs/toolchain@v1.0.6
       with:
-        toolchain: nightly
         target: x86_64-unknown-linux-gnu
         profile: minimal

@@ -247,7 +228,6 @@ jobs:
     - name: rust-toolchain
       uses: actions-rs/toolchain@v1.0.6
       with:
-        toolchain: nightly
         target: x86_64-unknown-linux-gnu
         profile: minimal

diff --git a/.github/workflows/clippy_dev.yml b/.github/workflows/clippy_dev.yml
index 5ee157cf2..8ca6a8c0c 100644
--- a/.github/workflows/clippy_dev.yml
+++ b/.github/workflows/clippy_dev.yml
@@ -25,10 +25,8 @@ jobs:
     - name: rust-toolchain
       uses: actions-rs/toolchain@v1.0.6
       with:
-        toolchain: nightly
         target: x86_64-unknown-linux-gnu
         profile: minimal
-        components: rustfmt

     - name: Checkout
       uses: actions/checkout@v2.3.3

flip1995 · 2020-12-09T18:03:48Z

^We also have to remove the toolchain: nightly, because otherwise it installs the latest nightly and on cargo build it will install the specified nightly, according to the actions-rs/toolchain documentation.

ebroto · 2020-12-09T18:04:49Z

^We also have to remove the toolchain: nightly, because otherwise it installs the latest nightly and on cargo build it will install the specified nightly, according to the actions-rs/toolchain documentation.

Hmm don't we need nightly for the fmt tests?

EDIT: I thought we wanted to keep that behavior in case rustfmt is missing in a particular nightly, since nightly itself can be changed to point to another one.

ebroto · 2020-12-09T18:09:52Z

@bors try

bors · 2020-12-09T18:10:00Z

⌛ Trying commit 8f95ae4 with merge ff3c4b1...

📌 Pin Clippy to a nightly 📌 changelog: none Addresses partially #5561. As proposed there in [this comment](#5561 (comment)), this kicks off the process, to help us get acquainted with how the syncs should work, before working on improving the tooling. Open questions: * When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that. * When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful. * Who does the rustups now? If we follow my previous idea and do both rustup and clippyup at the same time, it would be more work for `@flip1995` who currently does the clippyups. I would prefer to establish some kind of rotation to spead the work. Other ideas? * I'm not sure if this affects the release process in any way. * ??? `@rust-lang/clippy` thoughts? r? `@flip1995`

bors · 2020-12-10T09:09:00Z

⌛ Trying commit dd30c42 with merge b4f396c...

flip1995 · 2020-12-10T09:20:09Z

@bors try

Let's try to enable all of our integration tests again

bors · 2020-12-10T09:20:16Z

⌛ Trying commit 279721c with merge 7579123...

📌 Pin Clippy to a nightly 📌 changelog: Pin Clippy to a specific nightly version (No more master/custom toolchain required to compile Clippy) Addresses partially #5561. As proposed there in [this comment](#5561 (comment)), this kicks off the process, to help us get acquainted with how the syncs should work, before working on improving the tooling. Open questions: * When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that. * When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful. * Who does the rustups now? If we follow my previous idea and do both rustup and clippyup at the same time, it would be more work for `@flip1995` who currently does the clippyups. I would prefer to establish some kind of rotation to spead the work. Other ideas? * I'm not sure if this affects the release process in any way. * ??? `@rust-lang/clippy` thoughts? r? `@flip1995`

flip1995 · 2020-12-10T09:30:45Z

Uhm some changes in this PR make our integration test silently fail/not run at all. I'll investigate.

bors · 2020-12-10T09:31:48Z

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 7579123 (7579123a654f4a5d9428a433d6f877fa806b58fb)

The only thing we now cache is cargo-cache, which we only use for cache. That's a catch-22 if I ever seen one. And for Clippy itself we always want to do a clean build and not cache anything.

flip1995 · 2020-12-10T10:06:02Z

@bors try

bors · 2020-12-10T10:06:09Z

⌛ Trying commit d3b0a65 with merge ad89934...

📌 Pin Clippy to a nightly 📌 changelog: Pin Clippy to a specific nightly version (No more master/custom toolchain required to compile Clippy) Addresses partially #5561. As proposed there in [this comment](#5561 (comment)), this kicks off the process, to help us get acquainted with how the syncs should work, before working on improving the tooling. Open questions: * When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that. * When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful. * Who does the rustups now? If we follow my previous idea and do both rustup and clippyup at the same time, it would be more work for `@flip1995` who currently does the clippyups. I would prefer to establish some kind of rotation to spead the work. Other ideas? * I'm not sure if this affects the release process in any way. * ??? `@rust-lang/clippy` thoughts? r? `@flip1995`

bors · 2020-12-10T10:18:33Z

💔 Test failed - checks-action_test

flip1995 · 2020-12-10T11:47:15Z

@bors try (please work)

📌 Pin Clippy to a nightly 📌 changelog: Pin Clippy to a specific nightly version (No more master/custom toolchain required to compile Clippy) Addresses partially #5561. As proposed there in [this comment](#5561 (comment)), this kicks off the process, to help us get acquainted with how the syncs should work, before working on improving the tooling. Open questions: * When performing a rustup, we will need to exclude the commits that were merged that same day, or else wait until that nightly is released. I did not update the documentation about this part, mainly because I'm not sure about how to do that. * When should we perform the rustups now? My first idea is to do it at the same time we do the clippyups, to have a clear cadence and to avoid the two copies of the repo to diverge enough to make the process painful. * Who does the rustups now? If we follow my previous idea and do both rustup and clippyup at the same time, it would be more work for `@flip1995` who currently does the clippyups. I would prefer to establish some kind of rotation to spead the work. Other ideas? * I'm not sure if this affects the release process in any way. * ??? `@rust-lang/clippy` thoughts? r? `@flip1995`

bors · 2020-12-10T11:47:23Z

⌛ Trying commit 836325e with merge 092ff67...

bors · 2020-12-10T11:57:45Z

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 092ff67 (092ff6714c7df737212f96ca57d7ffd8be556613)

ebroto

LGTM in general (github won't let me auto-approve the PR 😄)

I just left a doubt.

ebroto · 2020-12-10T21:40:49Z

.github/workflows/clippy.yml

-    - name: Run cargo update
-      run: cargo update
+    - name: Install toolchain
+      run: rustup show active-toolchain


If we remove the rust-toolchain step, from where does the rustup used in this command come from?

GHA ships with stable rust+rustup already installed. See GHA documentation about the pre-installed software

That's convenient :)

BTW thanks for the work on this, you saved me another night fighting the CI 😄

NP, I like CI pain 😄

So can we merge this? :)

Sure! I was hesitant about approving a PR I opened 😄

ebroto · 2020-12-11T16:25:02Z

@bors r+

bors · 2020-12-11T16:25:04Z

📌 Commit 836325e has been approved by ebroto

bors · 2020-12-11T16:25:18Z

⌛ Testing commit 836325e with merge baf5f2d...

bors · 2020-12-11T16:37:10Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing baf5f2d to master...

rust-highfive assigned flip1995 Nov 29, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 29, 2020

ebroto marked this pull request as draft November 29, 2020 17:39

flip1995 reviewed Nov 29, 2020

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

setup-toolchain.sh Show resolved Hide resolved

phansch reviewed Nov 29, 2020

View reviewed changes

doc/basics.md Show resolved Hide resolved

ebroto marked this pull request as ready for review November 30, 2020 21:33

This comment has been minimized.

Sign in to view

flip1995 approved these changes Dec 6, 2020

View reviewed changes

ebroto force-pushed the pin_to_a_nightly branch from ba68518 to 82a4206 Compare December 9, 2020 16:21

ebroto force-pushed the pin_to_a_nightly branch from 82a4206 to 945536b Compare December 9, 2020 16:34

flip1995 added 2 commits December 10, 2020 10:53

Fix toolchain installation in workflows

41cab83

Stop caching on CI

26dcbf5

The only thing we now cache is cargo-cache, which we only use for cache. That's a catch-22 if I ever seen one. And for Clippy itself we always want to do a clean build and not cache anything.

flip1995 force-pushed the pin_to_a_nightly branch from 279721c to d3b0a65 Compare December 10, 2020 10:04

flip1995 added 2 commits December 10, 2020 12:44

Error in integration test, if required toolchain is not installed

3f41fe2

Fix integration test runner

836325e

flip1995 force-pushed the pin_to_a_nightly branch from d3b0a65 to 836325e Compare December 10, 2020 11:46

ebroto commented Dec 10, 2020

View reviewed changes

bors merged commit baf5f2d into rust-lang:master Dec 11, 2020

bors mentioned this pull request Dec 11, 2020

Remove rustfmt bug workaround #6083

Closed

ebroto deleted the pin_to_a_nightly branch December 11, 2020 16:38

xiongmao86 mentioned this pull request Dec 30, 2020

New lint: Use Into/TryInto in bounds as opposed to From/TryFrom #4894

Open

📌 Pin Clippy to a nightly 📌 #6401

📌 Pin Clippy to a nightly 📌 #6401

Conversation

ebroto commented Nov 29, 2020 • edited by flip1995 Loading

ebroto commented Nov 29, 2020

matthiaskrgr commented Nov 29, 2020

flip1995 commented Nov 29, 2020

flip1995 left a comment

Choose a reason for hiding this comment

matthiaskrgr commented Nov 29, 2020

ebroto commented Nov 29, 2020 • edited Loading

ebroto commented Nov 30, 2020

Manishearth commented Nov 30, 2020

flip1995 commented Dec 1, 2020

This comment has been minimized.

flip1995 left a comment

Choose a reason for hiding this comment

ebroto commented Dec 9, 2020

flip1995 commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 9, 2020

flip1995 commented Dec 9, 2020 • edited Loading

flip1995 commented Dec 9, 2020

flip1995 commented Dec 9, 2020

ebroto commented Dec 9, 2020 • edited Loading

ebroto commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 10, 2020

flip1995 commented Dec 10, 2020

bors commented Dec 10, 2020

flip1995 commented Dec 10, 2020

bors commented Dec 10, 2020

flip1995 commented Dec 10, 2020

bors commented Dec 10, 2020

bors commented Dec 10, 2020

flip1995 commented Dec 10, 2020

bors commented Dec 10, 2020

bors commented Dec 10, 2020

ebroto left a comment

Choose a reason for hiding this comment

ebroto Dec 10, 2020

Choose a reason for hiding this comment

flip1995 Dec 10, 2020

Choose a reason for hiding this comment

ebroto Dec 10, 2020

Choose a reason for hiding this comment

flip1995 Dec 11, 2020

Choose a reason for hiding this comment

ebroto Dec 11, 2020

Choose a reason for hiding this comment

ebroto commented Dec 11, 2020

bors commented Dec 11, 2020

bors commented Dec 11, 2020

bors commented Dec 11, 2020

ebroto commented Nov 29, 2020 •

edited by flip1995

Loading

ebroto commented Nov 29, 2020 •

edited

Loading

flip1995 commented Dec 9, 2020 •

edited

Loading

ebroto commented Dec 9, 2020 •

edited

Loading