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

Dogfood and CI fixes #6849

Merged
merged 6 commits into from
Mar 5, 2021
Merged

Dogfood and CI fixes #6849

merged 6 commits into from
Mar 5, 2021

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Mar 5, 2021

The CI fix is practically #6829 rebased and squashed into one commit

Dogfood fix is a follow up of #6802

r? @matthiaskrgr for lintcheck changes

(best reviewed with whitespace changes hidden)

changelog: none

Those workflows should always test exactly the same things
Since clippy cannot be a workspace, we have to check the sub-crates separately
This isn't necessary anymore, since we don't use a custom toolchain anymore
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 5, 2021
@flip1995
Copy link
Member Author

flip1995 commented Mar 5, 2021

@bors try

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

⌛ Trying commit c46a051 with merge 6ed2cd3...

bors added a commit that referenced this pull request Mar 5, 2021
Dogfood and CI fixes

The CI fix is practically #6829 rebased and squashed into one commit

Dogfood fix is a follow up of #6802

r? `@matthiaskrgr` for lintcheck changes

(best reviewed with whitespace changes hidden)

changelog: none
@flip1995
Copy link
Member Author

flip1995 commented Mar 5, 2021

This adds about 1m20s to the Test job which is expected, because now dogfood is actually run.

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 6ed2cd3 (6ed2cd3a1538fc9e97cc1e4636be36e2f98ee282)

Copy link
Member

@matthiaskrgr matthiaskrgr left a comment

Choose a reason for hiding this comment

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

r=me with the panic fixed

Comment on lines 584 to 600
let name = match krate {
CrateSource::CratesIo { name, .. } => name,
CrateSource::Git { name, .. } => name,
CrateSource::Path { name, .. } => name,
CrateSource::CratesIo { name, .. } | CrateSource::Git { name, .. } | CrateSource::Path { name, .. } => {
name
},
};
Copy link
Member

Choose a reason for hiding this comment

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

that's super neat, I'll have to remember that! :)

Copy link
Member Author

@flip1995 flip1995 Mar 5, 2021

Choose a reason for hiding this comment

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

This is an auto-fixable Clippy lint 😉

Comment on lines 117 to 133
println!("Downloading and extracting {} {} from {}", name, version, url);
let _ = std::fs::create_dir("target/lintcheck/");
let _ = std::fs::create_dir(&krate_download_dir);
let _ = std::fs::create_dir(&extract_dir);
std::fs::create_dir("target/lintcheck/").expect("cannot create lintcheck target dir");
std::fs::create_dir(&krate_download_dir).expect("cannot create crate download dir");
std::fs::create_dir(&extract_dir).expect("cannot create crate extraction dir");

Copy link
Member

Choose a reason for hiding this comment

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

This panics now because the directory may already exist (previously we just ignored the error)
I think we can work around by using std::fs::create_dir_all

let _ = std::fs::create_dir(&krate_download_dir);
let _ = std::fs::create_dir(&extract_dir);
std::fs::create_dir("target/lintcheck/").unwrap_or_else(|err| {
if err.kind() != ErrorKind::AlreadyExists {
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to only allow the dir to exist. I think panicking for other things should be fine?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, seems fine. :)

@flip1995
Copy link
Member Author

flip1995 commented Mar 5, 2021

@bors r=matthiaskrgr

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

📌 Commit 74eb448 has been approved by matthiaskrgr

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

⌛ Testing commit 74eb448 with merge e368355...

bors added a commit that referenced this pull request Mar 5, 2021
Dogfood and CI fixes

The CI fix is practically #6829 rebased and squashed into one commit

Dogfood fix is a follow up of #6802

r? `@matthiaskrgr` for lintcheck changes

(best reviewed with whitespace changes hidden)

changelog: none
@bors
Copy link
Collaborator

bors commented Mar 5, 2021

💔 Test failed - checks-action_test

@flip1995
Copy link
Member Author

flip1995 commented Mar 5, 2021

@bors retry (network error, the macOS runner seems to have problems recently.)

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

⌛ Testing commit 74eb448 with merge 9e54538...

@bors
Copy link
Collaborator

bors commented Mar 5, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: matthiaskrgr
Pushing 9e54538 to master...

@bors bors merged commit 9e54538 into rust-lang:master Mar 5, 2021
@flip1995 flip1995 deleted the dogfood-fix branch March 5, 2021 14:34
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.

4 participants