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.

Sign up for GitHub

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

Jump to bottom

Update rustc-perf submodule before running tidy #126225

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Update rustc-perf submodule before running tidy #126225

merged 1 commit into from
Jun 10, 2024

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jun 10, 2024 •
edited
Loading

Since #125465, tidy checks src/tools/rustc-perf, so we need to have it checked out before running tidy.

Fixes: #126224

r? @onur-ozkan

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 10, 2024
@lqd
Copy link
Member

lqd commented Jun 10, 2024

Will that require everyone to have the submodule checked out?

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 10, 2024

Yes, if they want to run tidy. Because it now checks the license of rustc-perf (since we now vendor it during dist).

@onur-ozkan
Copy link
Member

@bors r+ p=1 (tidy is broken atm)

@bors
Copy link
Contributor

bors commented Jun 10, 2024

📌 Commit faac70b has been approved by onur-ozkan

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2024
@onur-ozkan
Copy link
Member

Will that require everyone to have the submodule checked out?

Checking out a submodule is already required since #125465. This PR makes it done automatically.

@onur-ozkan
Copy link
Member

Another checkout is needed before vendoring (ref https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/rustc-perf.20workspace.20doesn't.20have.20a.20Cargo.2Elock/near/443741766)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 10, 2024
@onur-ozkan
Copy link
Member

Another checkout is needed before vendoring (ref https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/rustc-perf.20workspace.20doesn't.20have.20a.20Cargo.2Elock/near/443741766)

@bors r-

Seems like that's already done before vendoring.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 10, 2024

📌 Commit faac70b has been approved by onur-ozkan

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2024
@lqd
Copy link
Member

lqd commented Jun 10, 2024 •
edited
Loading

Will that require everyone to have the submodule checked out?

Checking out a submodule is already required since #125465. This PR makes it done automatically.

I don't find that argument super convincing tbh: opt-dist is a dist concern, not really something that contributors should be worrying about in their day-to-day work that doesn't involve it. In particular here, when the issue this fixes is not checked on CI.

@bors
Copy link
Contributor

bors commented Jun 10, 2024

⌛ Testing commit faac70b with merge 0de24a5...

@onur-ozkan
Copy link
Member

onur-ozkan commented Jun 10, 2024 •
edited
Loading

Checking out a submodule is already required since #125465. This PR makes it done automatically.

I don't find that argument super convincing tbh: opt-dist is a dist concern, not really something that contributors should be worrying about in their day-to-day work that doesn't involve it. In particular here, when the issue this fixes is not checked on CI.

#125465 made tidy to check the rustc-perf license. So now tidy fails if the submodule isn't checked out, and this PR checkouts the submodule instead of leaving tidy to fail.

I will improve this further by not checking rustc-perf license from tidy with a condition something like if not CI && submodule isn't checked out (which will make the tidy experience the same as before for devs) later today.

@bors
Copy link
Contributor

bors commented Jun 10, 2024

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 0de24a5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 10, 2024
@bors bors merged commit 0de24a5 into rust-lang:master Jun 10, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 10, 2024
@Kobzol Kobzol deleted the tidy-update-rustc-perf branch June 10, 2024 13:18
@onur-ozkan onur-ozkan mentioned this pull request Jun 10, 2024
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0de24a5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.707s -> 672.142s (-0.23%)
Artifact size: 319.82 MiB -> 319.14 MiB (-0.21%)

@jieyouxu jieyouxu mentioned this pull request Jun 16, 2024
Closed
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
@compiler-errors
Rollup merge of rust-lang#126230 - onur-ozkan:followup-126225, r=Mark…
ed0ba52 
…-Simulacrum

tidy: skip submodules if not present for non-CI environments

Right now tidy requires rustc-perf to be fetched as it checks its license, but this doesn't make sense for most contributors since rustc-perf is a dist-specific tool and its license will not change frequently, especially during compiler development. This PR makes tidy to skip rustc-perf if it's not fetched and if it's not running in CI.

Applies rust-lang#126225 (comment) and reverts rust-lang#126225.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
@compiler-errors
Rollup merge of rust-lang#126230 - onur-ozkan:followup-126225, r=Mark…
1410137 
…-Simulacrum

tidy: skip submodules if not present for non-CI environments

Right now tidy requires rustc-perf to be fetched as it checks its license, but this doesn't make sense for most contributors since rustc-perf is a dist-specific tool and its license will not change frequently, especially during compiler development. This PR makes tidy to skip rustc-perf if it's not fetched and if it's not running in CI.

Applies rust-lang#126225 (comment) and reverts rust-lang#126225.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 23, 2024
@matthiaskrgr
Rollup merge of rust-lang#126230 - onur-ozkan:followup-126225, r=Mark…
f016552 
…-Simulacrum

tidy: skip submodules if not present for non-CI environments

Right now tidy requires rustc-perf to be fetched as it checks its license, but this doesn't make sense for most contributors since rustc-perf is a dist-specific tool and its license will not change frequently, especially during compiler development. This PR makes tidy to skip rustc-perf if it's not fetched and if it's not running in CI.

Applies rust-lang#126225 (comment) and reverts rust-lang#126225.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
@rust-timer
Unrolled build for rust-lang#126230
2ce5258 
Rollup merge of rust-lang#126230 - onur-ozkan:followup-126225, r=Mark-Simulacrum

tidy: skip submodules if not present for non-CI environments

Right now tidy requires rustc-perf to be fetched as it checks its license, but this doesn't make sense for most contributors since rustc-perf is a dist-specific tool and its license will not change frequently, especially during compiler development. This PR makes tidy to skip rustc-perf if it's not fetched and if it's not running in CI.

Applies rust-lang#126225 (comment) and reverts rust-lang#126225.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 24, 2024
@matthiaskrgr
Rollup merge of #126230 - onur-ozkan:followup-126225, r=Mark-Simulacrum
ab223f2 
tidy: skip submodules if not present for non-CI environments

Right now tidy requires rustc-perf to be fetched as it checks its license, but this doesn't make sense for most contributors since rustc-perf is a dist-specific tool and its license will not change frequently, especially during compiler development. This PR makes tidy to skip rustc-perf if it's not fetched and if it's not running in CI.

Applies rust-lang/rust#126225 (comment) and reverts #126225.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@onur-ozkan onur-ozkan

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.

tidy error: rustc-perf workspace doesn't have a Cargo.lock
6 participants
@Kobzol @lqd @onur-ozkan @bors @rust-timer @rustbot