Why not to set rustflags empty by default?

[qalisander]

Awesome gh action! But one thing could be better as I feel.

The default behavior in complex pipelines can easy turn every pipeline into red.
At first glance it seems, that natural disaster happened. While in reality just a single warning appeared.
Isn't it better to have a single pipeline dedicated to a warning analysis? And have just a single use of rustflags property in this case. When that specific pipeline fails it is quite clear what happened.

IMHO default -D warnings overriding behavior of flags from config.toml looks quite tricky for new users.

[robjtede]

In the effort to promote sensible defaults, I feel that the positive impact of -Dwarnings far outweighs the negative given how easy it is to disable. CI is more often than not the place where you want to block contributions until code quality improvements have been made.

[jonasbb]

GitHub has decided on using two states for their CI results (pass/fail), but in Rust we have three (clean/warnings/errors), so something needs to be done. Personally, I see the risk of bit rot and hidden issues. Rust errors are quite good, with a low amount of false positives. As such they should be acted upon. Without denying warnings in CI, they will be hidden behind green pass mark for CI.

The default behavior in complex pipelines can easy turn every pipeline into red.
At first glance it seems, that natural disaster happened. While in reality just a single warning appeared.
Isn't it better to have a single pipeline dedicated to a warning analysis? And have just a single use of rustflags property in this case. When that specific pipeline fails it is quite clear what happened.

Is it a significant difference if one or multiple pipelines fail? After all, the result is a failed CI run and a red cross in either case. A separate warnings check also consumes extra CI resources, but you can do this with this action if you so want.

IMHO default -D warnings overriding behavior of flags from config.toml looks quite tricky for new users.

Yes, if you have a more demanding setup the defaults of this action might not work for you. I am fine with this action making some opinionated decisions.

[qalisander]

Got your point. Good to hear from you!

[qalisander]

Haven't you considered to add an argument to action named like "fail-on-warning", that will be true by default. Being true this flag can append -D warnings to default rustflags. Your expected behavior won't be changed.
But by the end preset rustflags won't be erased silently.
@jonasbb @robjtede wdt?

[jbg]

@qalisander the difficult part is "append … to default rustflags". This action has no way of knowing what your rustflags are going to be, because it doesn't have access to the repositories you're going to build. And the RUSTFLAGS env var overrides any rustflags set in .cargo/config.toml

[qalisander]

By rustflags I mean this NEW_RUSTFLAGS variable in the code.

By the end it should look like this for .cargo/config.toml use-case:

      - uses: actions-rust-lang/setup-rust-toolchain@v1
        with:
          fail-on-warning: false

Instead of:

      - uses: actions-rust-lang/setup-rust-toolchain@v1
        with:
          rustflags: ""

And when you want to set a flag explicitly and fail on warning it can look like this:

      - uses: actions-rust-lang/setup-rust-toolchain@v1
        with:
          rustflags: "-L rust_flag"

Instead of this:

      - uses: actions-rust-lang/setup-rust-toolchain@v1
        with:
          rustflags: "-L rust_flag -D warnings"

Not ideal though, but possibly better DevEx.

[jonasbb]

Haven't you considered to add an argument to action named like "fail-on-warning", that will be true by default.
In your examples, I don't really see a benefit of the proposed addition. It doesn't make unsetting rustflags any simpler. Instead, you now have two things to consider.

.cargo/config.toml are not considered so far. If that is necessary, maybe a more holistic approach is possible, instead of just the rustflags.

[lochsh]

I think the problem is that by default the compiler flags are overridden. It's not like -D warnings is by default appended to any compiler flags sourced from .cargo/config.toml, it just wipes them out.

Thankfully you can work around this by setting the rustflags argument to an empty string, but I assume it's not intended that non-empty values for the rustflags argument override any other flags rather than append to them?

It took me a while to figure out that this was the source of my project not building with position-independent relocations in CI, which breaks the code when it's used.

If you're opinionated about warnings being failures by default (I agree that makes a lot of sense for CI!) then maybe it can be appended instead of wiping out any other flags you have set.

[jonasbb]

Unfortunately, cargo does not really offer any way to have RUSTFLAGs being appended. There is an old issue about it: rust-lang/cargo#5376.
The only workaround seems to be a .cargo/config.toml file with a content like this, but I feel generating such feels is a problem.

[target.'cfg(all())']
rustflags = ["--cfg", "tokio_unstable"]

I saw that getrandom v0.3 recommends a .cargo/config.toml file for WASM support.

While I want to keep the -D warnings default, maybe it is necessary, to not meddle with any .cargo/config.toml files, either generally by the file just existing, or only if the config.toml actually specifies any rustflags = ... entries.

find . -path '*/.cargo/config.toml' -type f -exec grep 'rustflags *=' {} +

[jbg]

Note that for this to be reliable you would need to search both possible filenames (config.toml and config) in all paths back up to the root directory, as well in $CARGO_HOME. https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure

[mcandre]

In the effort to promote sensible defaults, I feel that the positive impact of -Dwarnings far outweighs the negative given how easy it is to disable. CI is more often than not the place where you want to block contributions until code quality improvements have been made.

I publish linters. I enable all possible compiler warnings and treat them as errors. Expecting that new rules in the future will break my builds. But I don't think it's a good idea to change stock configuration in CI/CD for a provisioning script intended to simply install the language. As a user I would never expect opinionated secret sauce there. If I want a different configuration I'll apply it myself. Or write an extension GitHub Action workflow with supplemental configurations.

Setting aside the confusing, unexpected behavior, unsetting environment variables is downstream scripts is more difficult than customizing. Many CI/CD jobs have multiple commands. Don't violate DRY by forcing us to repeat the unset logic per-step.

If anyone is concerned with backwards compatibility then let's bump the workflow major.

[jonasbb]

But I don't think it's a good idea to change stock configuration in CI/CD for a provisioning script intended to simply install the language.

This action is slightly opinionated in how it sets up everything. There are other environment variables being set besides RUSTFLAGS and the action also sets up caching. If you want a pure language installation script you don't event need to do anything on the GitHub hosted runners, since Rust is already pre-installed. https://github.com/dtolnay/rust-toolchain/ is also a good choice for a simple language installation action.