rustPlatform.cargoSetupHook: allow all lints

[winterqt]

We've had multiple occurrences of packages that don't compile after a rustc bump due to new lints. Let's stop this once and for all, by using rustc's lint capping feature 0.

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin, nix-build -A fd
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[winterqt]

Maybe "allow all lints when compiling" is better.

[figsoda]

Maybe "allow all lints when compiling" is better.

+1 for me, but does that mean it would have to be in all of cargoBuildHook, cargoCheckHook and cargoNextestHook?

[winterqt]

I'm operating under the assumption that people will be dragging in cargoSetupHook if they're doing anything else -- is that a bad assumption?

[figsoda]

no I think the assumption is good, I thought you were worried that having this in cargoSetupHook will allow all lints even when not compiling

[figsoda]

Another worry I have is this might overwrite rustflags settings in the toml config, see #221413

[winterqt]

no I think the assumption is good, I thought you were worried that having this in cargoSetupHook will allow all lints even when not compiling

Oh, I was just talking about the commit message, hence the quotes.

cc @alyssais for the TOML stuff -- should we be setting this somewhere else?

[oxalica]

Not a fan of this.

We've had multiple occurrences of packages that don't compile after a rustc bump due to new lints.

Could you paste some links of them?

To me,

1. deny-by-default lints usually mean unexpected or (accidentally stabilized) undefined behavior, which should be seen as alerts for behavior bugs. If this occurs, we need to investigate the particular issue and probably patch related code.
2. Suppressing them by default loses the chance to catch issues during auto-update build tests.
3. RUSTFLAGS's priority is too high and can break other project specific setup, as mentioned, rust: fix overriding rust flags on musl #221413
4. #![deny(warnings)] is already an anti-pattern and should be unusual.

[winterqt]

We've had multiple occurrences of packages that don't compile after a rustc bump due to new lints.

Could you paste some links of them?

See all the cherry-picked commits in #213287 as an example, and I know @vcunat sees this regularly.

[alyssais]

Is there a way we could disable #![deny(warnings)] without disabling the deny-by-default lints @oxalica mentions? If we could, then I think that would be cool.

Regardless, we definitely shouldn't use RUSTFLAGS to do it, but maybe there's some other way we could do it. And we could definitely do it when we have the much-anticipated Rust compiler wrapper that everybody seems to have accepted is coming eventually.

[oxalica]

Is there a way we could disable #![deny(warnings)] without disabling the deny-by-default lints @oxalica mentions?

--cap-lints=warn is slightly better than --cap-lints=allow, if we do want to ignore them anyway. --cap-lints=warn will still print warnings when it needs to, but won't block the complication.

Regardless, we definitely shouldn't use RUSTFLAGS to do it, but maybe there's some other way we could do it.

I find this env var: CARGO_BUILD_RUSTFLAGS='--cap-lints=warn'

• It behaves like a virtual .config/config.toml with [build] rustflags =.
• It merges with the real .config/config.toml .
• It has lower priority than RUSTFLAGS so it can be overridden when necessary.

[alyssais]

I was just talking to @estebank,who's on the Rust compiler team. He says that he doesn't think there's currently a flag that would allow us to override #![deny(warnings)], but that one could potentially be added and we could file a ticket.

So, I'd like to reframe the discussion here a little bit: leaving aside what's currently possible with the Rust tooling, what would our ideal situation be? Would it be to drop denials from crates, but leave default denials from the compiler intact, or would it be something else?

[oxalica]

what would our ideal situation be?

I'm lean towards not ignoring this globally. When we encountering a deny(warnings) bomb, PR the upstream to remove the future-incompatibility friction, by maybe moving it into their CI. If the upstream does not cooperating, we either patch it (just one-line substitution!), or toggle the flag package-wise. In other words, I treat deny(warnings) as an upstream issue, not a packaging issue.

[winterqt]

I think we've come to a consensus that this isn't the best way forward. I guess I can file a ticket on the Rust side like @alyssais mentioned.

[estebank]

Amongst some people in the Rust side, we've had discussions about annotating lints with metadata on which version they were introduced/most recently significantly modified, and then providing a way to cap lints in a version aware way. This would allow a project to say "deny all lints that existed on rust 1.90 (but hard cap any lints introduced after that to warn only)". I believe this feature would work for your usecase. Sadly, I don't think I've filed an MCP/RFC for this yet and don't know how quickly the feature could be stabilized. If someone is interested on doing the work, I can help by mentoring through the process of proposing and implementing it.