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

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

Merged
merged 2 commits into from
Jun 23, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jun 10, 2024

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 #126225 (comment) and reverts #126225.

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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
@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

// Skip if it's a submodule, not initialized, and not in a CI environment.
//
// This prevents enforcing developers to fetch submodules for tidy.
if is_submodule && !root.join(workspace).join(".git").exists() && !CiEnv::is_ci() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This won't work in tarball sources, will change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to check if the directory is empty or not. For non-tarball sources, it's not possible to have an initialized submodule as an empty directory (because .git will be present). Even if it's a tarball source, we don't have any tool that is a submodule but an empty directory anyway. We can do this check this by using git, but it makes things overly complicated and it wouldn't work on tarball sources.

src/tools/tidy/src/deps.rs Outdated Show resolved Hide resolved
//("src/etc/test-float-parse", &[], None), // FIXME uncomment once all deps are vendored
("src/tools/cargo", EXCEPTIONS_CARGO, None),
("src/tools/cargo", EXCEPTIONS_CARGO, None, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite clear why this is false. src/tools/cargo is a submodule.

Overall I'm confused by this PR. tidy has depended on submodules existing for some time. From my understanding, things like the cargo submodule implicitly work via this check which ensures a subset of submodules are available.

Why not just leave #126225 (or some variation of that), and always check rustc-perf? Is there some significant downside of cloning rustc-perf?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lqd It seemed like you didn't like that we would force contributors to download rustc-perf. Do you have any specific concerns? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Overall I'm confused by this PR. tidy has depended on submodules existing for some time. From my understanding, things like the cargo submodule implicitly work via this check which ensures a subset of submodules are available.

Fetching a dist specific tool and continuously checking its license with tidy on development doesn't seem like a good idea overall, at least in my opinion (I am pretty sure some of the active contributors like @RalfJung don't like this kind of approach either).

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'm not quite clear why this is false. src/tools/cargo is a submodule.

Added automatic submodule detection to prevent issues like this in the future.

cc @Kobzol

Copy link
Member

@RalfJung RalfJung Jun 10, 2024

Choose a reason for hiding this comment

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

I think ideally we'd require as few submodules as possible to be checked out. Submodules are annoying as updating them is always a separate step -- even if x.py does it automatically (which currently fails badly for me), the checkout is in a somewhat degraded state until the next x.py invocation.

If we can get that number down to 0, that would be perfect. :) But currently that's probably not yet possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine if there is a significant burden. Would it be possible to update the PR description to explain what the PR changes and the motivation for the change? That way, we're at least clear as to why it is changing (such as "avoids a significant download not relevant to most contributors", or whatever the actual reason is).

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR description.

@@ -515,6 +517,19 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) {
let mut checked_runtime_licenses = false;

for &(workspace, exceptions, permitted_deps) in WORKSPACES {
let gitmodules = root.join(".gitmodules");
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this into the function? It seems like the function could take the directory rather than the file path, which simplifies it and avoids duplicating the exact filename everywhere.

@@ -14,6 +15,19 @@ const ALLOWED_SOURCES: &[&str] = &[
/// workspace `Cargo.toml`.
pub fn check(root: &Path, bad: &mut bool) {
for &(workspace, _, _) in crate::deps::WORKSPACES {
let gitmodules = root.join(".gitmodules");
let submodules = build_helper::util::parse_gitmodules(&gitmodules);
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this out of the loop? No need to re-parse for every workspace.

Same with the other call site in deps.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2024
@onur-ozkan
Copy link
Member Author

Applied the review notes above.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 23, 2024

📌 Commit e9e3c38 has been approved by Mark-Simulacrum

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 23, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
…-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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126230 (tidy: skip submodules if not present for non-CI environments)
 - rust-lang#126523 (std: refactor the TLS implementation)
 - rust-lang#126612 (Update outdated README in build-manifest.)
 - rust-lang#126616 (less bootstrap warnings)
 - rust-lang#126663 (remove `GIT_DIR` handling in pre-push hook)
 - rust-lang#126830 (make unsized_fn_params an internal feature)
 - rust-lang#126833 (don't ICE when encountering an extern type field during validation)
 - rust-lang#126837 (delegation: Do not crash on qpaths without a trait)
 - rust-lang#126851 (Rework pattern and expression nonterminal kinds.)

r? `@ghost`
`@rustbot` modify labels: rollup
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
…-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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126230 (tidy: skip submodules if not present for non-CI environments)
 - rust-lang#126608 (Add more constants, functions, and tests for `f16` and `f128`)
 - rust-lang#126612 (Update outdated README in build-manifest.)
 - rust-lang#126616 (less bootstrap warnings)
 - rust-lang#126663 (remove `GIT_DIR` handling in pre-push hook)
 - rust-lang#126830 (make unsized_fn_params an internal feature)
 - rust-lang#126833 (don't ICE when encountering an extern type field during validation)
 - rust-lang#126837 (delegation: Do not crash on qpaths without a trait)
 - rust-lang#126851 (Rework pattern and expression nonterminal kinds.)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126230 (tidy: skip submodules if not present for non-CI environments)
 - rust-lang#126612 (Update outdated README in build-manifest.)
 - rust-lang#126616 (less bootstrap warnings)
 - rust-lang#126663 (remove `GIT_DIR` handling in pre-push hook)
 - rust-lang#126830 (make unsized_fn_params an internal feature)
 - rust-lang#126833 (don't ICE when encountering an extern type field during validation)
 - rust-lang#126837 (delegation: Do not crash on qpaths without a trait)
 - rust-lang#126851 (Rework pattern and expression nonterminal kinds.)
 - rust-lang#126862 (Add needs-symlink directive to compiletest)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f016552 into rust-lang:master Jun 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
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.
@onur-ozkan onur-ozkan deleted the followup-126225 branch June 24, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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
Development

Successfully merging this pull request may close these issues.

7 participants