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

Make --cap-lints and related options leave crate hash alone #87145

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented Jul 14, 2021

Closes: #87144

@rust-highfive
Copy link
Collaborator

r? @nagisa

(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 Jul 14, 2021
@jsgf jsgf force-pushed the fix-lint-opt-hash branch from 2fb8518 to 51142a0 Compare July 15, 2021 00:05
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I don't feel confident approving.

@jyn514
Copy link
Member

jyn514 commented Jul 15, 2021

r? @michaelwoerister

@est31
Copy link
Member

est31 commented Jul 15, 2021

How eagerly are lints being checked? As in, if say lints are capped to allow, do the lints still get checked? Because if they don't, this might affect internal data structures, which in turn might affect details in the output.

@jyn514
Copy link
Member

jyn514 commented Jul 15, 2021

How eagerly are lints being checked? As in, if say lints are capped to allow, do the lints still get checked?

Yes. #74704

@jsgf
Copy link
Contributor Author

jsgf commented Jul 15, 2021

@est31 You mean in the rlib, or in the incremental db?

@est31
Copy link
Member

est31 commented Jul 15, 2021

@jsgf both actually? It might query things at a different point in time, which can influence how things are stored in internal caches. IIRC the rlib format just dumps many such internal data structures, but I'm not 100% sure. There should be no functional difference, but the resulting rlib files might have different hashes. This question is better answered by someone who knows this better than me.

@jsgf
Copy link
Contributor Author

jsgf commented Jul 15, 2021

@est31 I see - having the rlib bitwise dependent on warning flags would certainly be very surprising. It doesn't seem to be the case for the limit tests I tried, but obv that's not a proof. But either way, if the crates are semantically identical then they can have the same crate hash even if the files are different bitwise (for example, -Zno-codegen generates a very different crate contents, but it has the same SVH).

cc @bjorn3 @Aaron1011 who have context from the last PR I put up along these lines.

@est31
Copy link
Member

est31 commented Jul 15, 2021

Mhh the -Zno-codegen point is a very good one. Maybe this isn't a problem then after all :).

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

If running lints causes the build output (except diagnostics) to change that would be a bug IMO. Lints should only look and not touch anything. If cache state would affect the build output, that would be a bug in the incremental compilation system that may be exposed even without this PR.

@michaelwoerister
Copy link
Member

Thanks for the PR, @jsgf! I agree with @bjorn3's assessment.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 15, 2021

📌 Commit 51142a0 has been approved by michaelwoerister

@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 Jul 15, 2021
This was referenced Jul 15, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#86983 (Add or improve natvis definitions for common standard library types)
 - rust-lang#87069 (ExprUseVisitor: Treat ByValue use of Copy types as ImmBorrow)
 - rust-lang#87138 (Correct invariant documentation for `steps_between`)
 - rust-lang#87145 (Make --cap-lints and related options leave crate hash alone)
 - rust-lang#87161 (RFC2229: Use the correct place type)
 - rust-lang#87162 (Fix type decl layout "overflow")
 - rust-lang#87167 (Fix sidebar display on small devices)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c1b9bbf into rust-lang:master Jul 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 16, 2021
@jsgf jsgf deleted the fix-lint-opt-hash branch July 16, 2021 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

--cap-lints and related affect crate hash
9 participants