Skip to content

add enable-warnings flag for llvm, and disable it by default. #108991

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
Merged
merged 1 commit into from
Mar 15, 2023
Merged

add enable-warnings flag for llvm, and disable it by default. #108991

merged 1 commit into from
Mar 15, 2023

Conversation

@onur-ozkan
Copy link
Contributor

This flag allows to turn off warnings of llvm compilation for people who are not interested on those warnings.

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@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 Mar 10, 2023
onur-ozkan
onur-ozkan commented Mar 10, 2023
View reviewed changes
onur-ozkan
onur-ozkan commented Mar 10, 2023
View reviewed changes
src/bootstrap/native.rs Outdated
Comment on lines -493 to -490
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/extending-llvm-variables

@onur-ozkan onur-ozkan changed the title add enable-warnings flag for llvm add enable-warnings flag for llvm and update llvm defaults of config.user.toml Mar 10, 2023
@onur-ozkan
Copy link
Contributor Author

I think you might be interested looking 2nd commit of this PR @jyn514

jyn514
jyn514 reviewed Mar 10, 2023
View reviewed changes
@onur-ozkan onur-ozkan changed the title add enable-warnings flag for llvm and update llvm defaults of config.user.toml add enable-warnings flag for llvm Mar 10, 2023
@albertlarsan68
Copy link
Member

I think the codegen profile, and at least some parts of CI will want this flag enabled.
cc @rust-lang/infra Do you have an opinion about this flag in CI?

@onur-ozkan
Copy link
Contributor Author

I think the codegen profile, and at least some parts of CI will want this flag enabled. cc @rust-lang/infra Do you have an opinion about this flag in CI?

Agree for codegen, but I am not sure about that CI checking warnings

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

This PR changes src/bootstrap/defaults/config.codegen.toml. If appropriate, please also update config.compiler.toml so the defaults are in sync.

@klensy
Copy link
Contributor

klensy commented Mar 10, 2023

I'm misreading, or it should be allows to turn *on* warnings, because it will be off by default?

@onur-ozkan
Copy link
Contributor Author

I'm misreading, or it should be allows to turn *on* warnings, because it will be off by default?

Currently, it's always on. With this PR, we are able to turn it off. The explanation doesn't target the default value or how the flag works. It explains what this PR brings.

@bors
Copy link
Collaborator

bors commented Mar 13, 2023

☔ The latest upstream changes (presumably #109056) made this pull request unmergeable. Please resolve the merge conflicts.

@albertlarsan68
Copy link
Member

Can you add an entry to the changelog please (it is located in src/bootstrap/CHANGELOG.md IIRC).
@rustbot author

@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 Mar 15, 2023
@onur-ozkan
Copy link
Contributor Author

@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 Mar 15, 2023
@onur-ozkan
add enable-warnings flag for llvm
2e7249f 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@albertlarsan68
Copy link
Member

Thanks for the PR!
@bors rollup
r=me once CI green

@albertlarsan68
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 15, 2023

📌 Commit 2e7249f has been approved by albertlarsan68

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 Mar 15, 2023
@klensy
Copy link
Contributor

klensy commented Mar 15, 2023 •
edited
Loading

I'm misreading, or it should be allows to turn *on* warnings, because it will be off by default?

Currently, it's always on. With this PR, we are able to turn it off. The explanation doesn't target the default value or how the flag works. It explains what this PR brings.

Currently, it's always on.

And now it will be default off, or I'm wrong (maybe i read bootstrap code wrong)?

config.llvm_enable_warnings = llvm.enable_warnings.unwrap_or(false);

Oh,

Add llvm option `enable-warnings` to have control on llvm compilation warnings. Default to false.

so it is changes defaults.

@albertlarsan68
Copy link
Member

The warnings will be off by default, except when using the codegen profile, because then warnings can be useful.

@klensy
Copy link
Contributor

klensy commented Mar 15, 2023

The warnings will be off by default, except when using the codegen profile, because then warnings can be useful.

Yes, but title says only about adding ability to switch warning, not changing it's current behavior (see my first message).

@albertlarsan68
Copy link
Member

The title is not the source of truth.
See the changelog and documentation for the option to get the true meaning and things changed.
That said, I will change the title, to avoid future confusion.

@albertlarsan68 albertlarsan68 changed the title add enable-warnings flag for llvm add enable-warnings flag for llvm, and enable it by default. Mar 15, 2023
@albertlarsan68 albertlarsan68 changed the title add enable-warnings flag for llvm, and enable it by default. add enable-warnings flag for llvm, and disable it by default. Mar 15, 2023
@onur-ozkan
Copy link
Contributor Author

The warnings will be off by default, except when using the codegen profile, because then warnings can be useful.

Yes, but title says only about adding ability to switch warning, not changing it's current behavior (see my first message).

You can't change the behaviour of llvm compilation warnings without the changes this PR provides. PR doesn't just turn off warnings automatically. It gives the ability of switching llvm compilation warnings on and off.

@Dylan-DPC Dylan-DPC mentioned this pull request Mar 15, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2023
@bors
Auto merge of rust-lang#109164 - Dylan-DPC:rollup-0bwxwos, r=Dylan-DPC
a167cbd 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108991 (add `enable-warnings` flag for llvm, and disable it by default.)
 - rust-lang#109109 (Use `unused_generic_params` from crate metadata)
 - rust-lang#109111 (Create dirs for build_triple)
 - rust-lang#109136 (Simplify proc macro signature validity check)
 - rust-lang#109150 (Update cargo)
 - rust-lang#109154 (Fix MappingToUnit  to support no span of arg_ty)
 - rust-lang#109157 (Remove mw from review rotation for a while)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c11399b into rust-lang:master Mar 15, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 15, 2023
@onur-ozkan onur-ozkan deleted the new-llvm-flag branch March 15, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albertlarsan68 albertlarsan68 albertlarsan68 left review comments

+1 more reviewer

@jyn514 jyn514 jyn514 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@albertlarsan68 albertlarsan68

Labels

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.70.0

Development

Successfully merging this pull request may close these issues.

6 participants

@onur-ozkan @rustbot @albertlarsan68 @klensy @bors @jyn514