Skip to content

ente-auth: patch out Werror#451301

Closed
iedame wants to merge 1 commit intoNixOS:masterfrom
iedame:update/ente-auth
Closed

ente-auth: patch out Werror#451301
iedame wants to merge 1 commit intoNixOS:masterfrom
iedame:update/ente-auth

Conversation

@iedame
Copy link
Contributor

@iedame iedame commented Oct 12, 2025

As per the recommendation from @SuperSandro2000 in this comment: #449770 (comment) , patching out Werror is the preferred method for handling this error.

This PR introduces those changes to patch out Werror for ente-auth, presenting us warnings instead of failing build.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Oct 12, 2025
@iedame
Copy link
Contributor Author

iedame commented Oct 12, 2025

nixpkgs-review result

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 451301
Commit: 29be045a72469c9b8633ee4aaecbfc7958a014c4 (subsequent changes)
Merge: 0f7b4dc17a55d94ef3d0e66a8a599cdbe4a934a9

Logs: https://github.com/iedame/nixpkgs-review-gha/actions/runs/18442907122


x86_64-linux

✅ 3 packages built:
  • ente-auth
  • ente-auth.debug
  • ente-auth.pubcache

aarch64-linux

✅ 3 packages built:
  • ente-auth
  • ente-auth.debug
  • ente-auth.pubcache

x86_64-darwin

No rebuilds


aarch64-darwin

No rebuilds

Copy link
Member

@niklaskorz niklaskorz left a comment

Choose a reason for hiding this comment

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

This is disabling Werror for ente-auth itself, not just the dependency.

@iedame
Copy link
Contributor Author

iedame commented Oct 12, 2025

This is disabling Werror for ente-auth itself, not just the dependency.

Any suggestions for disabling it only for the flutter_secure_storage_linux?
(The linked comment is from a similar error in another project, both use flutter_secure_storage_linux)

Related issue:
juliansteenbakker/flutter_secure_storage#965
Not sure what would be the best long term solution

@qzylinra
Copy link
Contributor

You should fix the code instead of disabling Werror. You can modify the code using customSourceBuilders or modify the https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/compilers/dart/package-source-builders/flutter-secure-storage-linux/default.nix file

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Oct 13, 2025

This is disabling Werror for ente-auth itself, not just the dependency.

Less Werror in projects means less churn on updates and staging runs. 👍🏼

You should fix the code instead of disabling Werror.

Upstream should fix their code but having Werror in nixpkgs just breaks projects on gcc updates and creates useless churn. Werror is a development flag and most warnings related around type annotations and other subtle c/c++ things which a none c/c++ developer cannot fix realistically.

I mean in the merged patch the warning was also just disabled which is equal to just turning off Werror.

@niklaskorz
Copy link
Member

Less Werror in projects means less churn on updates and staging runs. 👍🏼

I've reached out to the nixpkgs core team for a stance on this matter, I'm willing to go in either direction as long as we uniformly document and enforce it.

@UlyssesZh
Copy link
Member

I've reached out to the nixpkgs core team for a stance on this matter, I'm willing to go in either direction as long as we uniformly document and enforce it.

Maybe open a tracking issue?

@iedame
Copy link
Contributor Author

iedame commented Oct 18, 2025

Less Werror in projects means less churn on updates and staging runs. 👍🏼

I've reached out to the nixpkgs core team for a stance on this matter, I'm willing to go in either direction as long as we uniformly document and enforce it.

Any stance regarding this? #451855 have a similar issue. So far only server-box, ente-auth and the linked PR are using the -Wno-deprecated-literal-operator flag. I'm also willing to go in either direction, I completely understand the hesitation in disabling Werror but at same time I understand the advantages of having less churn and build failures on updates.

@emilazy
Copy link
Member

emilazy commented Oct 21, 2025

Sorry for the slow response from the Nixpkgs core team; Alyssa has been sick and we’re getting back into things now. We are discussing this and I hope to have a team reply in the next couple days.

@emilazy
Copy link
Member

emilazy commented Oct 25, 2025

Sorry for the belated response! We agreed on the following:

  • It’s okay to give people advice on how to improve their next contributions after a PR is already merged, but we think retroactive reviews that ask contributors to do more work after a PR was already merged should be reserved for fundamental concerns about the approach, significant regressions, or procedural failures (e.g. merge of a contribution that violates our documented guidelines).

    We can (and should) revert PRs that cause significant problems if they’re not addressed quickly, but it’s frustrating for contributors to get through the review process and then be expected to do more work for minor issues. When there are concerns to be raised, we think it’s better for them to be addressed primarily to the person who reviewed and took responsibility for merging a change.

  • Where compiler warnings identify real issues, we strongly prefer fixing them upstream to minimize future maintenance burden. We recognize that this is not always easy, though, and that working around them downstream with flags is sometimes necessary.

  • There is a genuine question of maintenance burden in how we handle these flags, so we think it’s an acceptable matter for review and don’t consider it a purely stylistic manner. (In the case of this flag, however, it’s not clear that patching out -Werror reduces maintenance burden over the previous approach; see below for details.)

  • However, if people desire consistency as to the method used to disabling errors, it’s generally best to open a PR to the contributor documentation to get consensus on the best approach so that we can have a standard for contributors and reviewers to reference. We can make a team judgement call on such PRs if necessary, although of course it’s always best to seek consensus instead.


Speaking more for myself here, in this case, it’s not clear that patching out -Werror will reduce maintenance burden over using -Wno-… or -Wno-error=…. The Clang 20 release notes say:

  • The warning -Wdeprecated-literal-operator is now on by default, as this is something that WG21 has shown interest in removing from the language.

Generally, when a language feature is deprecated, it first becomes an opt‐in warning, then a default warning, then an error‐by‐default warning, and potentially a hard error in future standard versions.

When it transitions from warning by default to erroring by default, -Wno-… and -Wno-error=… would keep the build working, but notably patching out -Werror wouldn’t: -Werror only affects things that warn by default. So, in this case, if the issue is not addressed upstream, and Clang 23 turns it into an error, the same problem would crop back up again unless we disable this warning specifically.

I think the maintenance burden depends on what we expect from upstreams: if we think they’ll fix it before it becomes a hard error, when they upgrade their compiler and run into the -Werror, then it won’t require handling again; dealing with it now by disabling -Werror is safe, and we wouldn’t end up potentially carrying a list of redundant flags to disable deprecations indefinitely. On the other hand, if we expect them to lag behind, then -Wno-error=… or -Wno-… will make sure it doesn’t crop back up. Clang 20 came out in March, so any upstream that hasn’t fixed this is at least an LLVM release cycle behind dealing with new warnings.

(We don’t have any immediate team consensus opinion on the ideal way to handle -Werror. I personally prefer sticking as close as possible to the upstream warning configuration and doing tightly‐scoped -Wno-error=…; Alyssa prefers disabling -Werror by default and reserving the granular flags for cases of gradual deprecation like in this PR. But discussing that further would be a community matter on a PR to document a standard approach.)

@niklaskorz
Copy link
Member

Thank you for taking the time to write such a thorough response! Until there is a different nixpkgs wide consensus, I will say that I as a package maintainer prefer to selectively disable warnings. So as the maintainer of this package, considering that the build issue itself was already fixed by the prior approach, I will close this PR.

@niklaskorz niklaskorz closed this Oct 25, 2025
@iedame iedame deleted the update/ente-auth branch October 25, 2025 18:04
@iedame
Copy link
Contributor Author

iedame commented Oct 25, 2025

Thank you to @emilazy and the Nixpkgs core team for taking the time to clarify this. Your explanation was very helpful in resolving this issue.

I hope this thorough response can be documented somewhere easily accessible for future contributors who encounter similar situations.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixpkgs-core-team-update-2025-11-04/71742/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants