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

Wire up tidy dependency checks for cg_clif #84872

Merged
merged 4 commits into from
May 7, 2021
Merged

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented May 3, 2021

Also contains a fix and improvement to tidy.

Required for #81746.

bjorn3 added 4 commits May 3, 2021 18:41
The comment says that build dependencies shouldn't matter unless they do
some kind of codegen. It is safer to always check it though.
@bjorn3 bjorn3 added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-cranelift Things relevant to the [future] cranelift backend labels May 3, 2021
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2021
@@ -44,12 +44,29 @@ const EXCEPTIONS: &[(&str, &str)] = &[
("fortanix-sgx-abi", "MPL-2.0"), // libstd but only for `sgx` target
];

const EXCEPTIONS_CRANELIFT: &[(&str, &str)] = &[
("cranelift-bforest", "Apache-2.0 WITH LLVM-exception"),
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 license is used by LLVM too, so it shouldn't be a problem.

Comment on lines +58 to +59
("libloading", "ISC"),
("mach", "BSD-2-Clause"),
Copy link
Member Author

Choose a reason for hiding this comment

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

Both of these licenses are equivalent to MIT I believe, but IANAL.

@joshtriplett
Copy link
Member

Would it potentially make sense to deduplicate the PERMITTED_CRANELIFT_DEPENDENCIES list with the PERMITTED_DEPENDENCIES list, and just add comments (or actual enum-based data types perhaps) to the cranelift-only ones? I think that'd make the list much easier to maintain, and in particular, it would make sure people don't let the lists get out of sync.

I'm thinking of two cases here:

  • Someone adds a dependency, and it'll definitely apply to all backends, but they don't realize they need to apply it to the cranelift list, so CI fails and they end up having to fix that later.
  • Someone removes a dependency from one list, but it's actually no longer needed for any backend.

@bjorn3
Copy link
Member Author

bjorn3 commented May 3, 2021

There are two lists as there are two workspaces. Changing a single Cargo.toml to add a dependency will only affect a single workspace and thus only one of the lists. The reason I use separate lists is because if I merge the lists, tidy will complain for both workspaces that there are exceptions that don't exist in the workspace that is currently being checked due to it only existing in the other workspace. This check also means that you can't forget to remove an exception fron either list once it is no longer necessary.

@joshtriplett
Copy link
Member

@bjorn3 Would it be feasible to merge the lists into one list that tags each dependency as needed for one, the other, or both? That seems like it'd simplify maintenance, and avoid additional CI cycles ("oops, I forgot to fix the backend I'm not using").

@bjorn3
Copy link
Member Author

bjorn3 commented May 3, 2021

Running tidy will always check both the main workspace and cg_clif's workspace. Merging both lists doesn't change anything about that. The only change will be that it makes the list a bit harder to read I think. There is barely any overlap in dependencies between rustc and cg_clif anyway.

@joshtriplett
Copy link
Member

@bjorn3 Ah, I see; I didn't realize that both lists would always get checked. In that case, this isn't a semantic issue, only a question of what's easier to maintain.

Sorry for the noise, then.

@Mark-Simulacrum
Copy link
Member

@bors r+

I think this PR is pretty much a strict improvement, though I want to be clear that this is not a confirmation of the license exceptions being OK - we'll want to evaluate those when we get closer to distributing cranelift, but for now this PR is still just an improvement of the status quo. Thanks!

@bors
Copy link
Contributor

bors commented May 6, 2021

📌 Commit 24def63 has been approved by Mark-Simulacrum

@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 May 6, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
Wire up tidy dependency checks for cg_clif

Also contains a fix and improvement to tidy.

Required for rust-lang#81746.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
Wire up tidy dependency checks for cg_clif

Also contains a fix and improvement to tidy.

Required for rust-lang#81746.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#84409 (Ensure TLS destructors run before thread joins in SGX)
 - rust-lang#84500 (Add --run flag to compiletest)
 - rust-lang#84728 (Add test for suggestion to borrow unsized function parameters)
 - rust-lang#84734 (Add `needs-unwind` and beginning of support for testing `panic=abort` std to compiletest)
 - rust-lang#84755 (Allow using `core::` in intra-doc links within core itself)
 - rust-lang#84871 (Disallows `#![feature(no_coverage)]` on stable and beta (using standard crate-level gating))
 - rust-lang#84872 (Wire up tidy dependency checks for cg_clif)
 - rust-lang#84896 (Handle incorrect placement of parentheses in trait bounds more gracefully)
 - rust-lang#84905 (CTFE engine: rename copy → copy_intrinsic, move to intrinsics.rs)
 - rust-lang#84953 (Remove unneeded call to with_default_session_globals in rustdoc highlight)
 - rust-lang#84987 (small nits)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 746f97e into rust-lang:master May 7, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 7, 2021
@bjorn3 bjorn3 deleted the cg_clif_tidy branch May 8, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cranelift Things relevant to the [future] cranelift backend C-enhancement Category: An issue proposing an enhancement or a PR with one. 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