config: make allowBroken actully allow everything marked as broken#534854
config: make allowBroken actully allow everything marked as broken#534854jopejoe1 wants to merge 1 commit into
allowBroken actully allow everything marked as broken#534854Conversation
nixpkgs-ci
Bot
added
10.rebuild-darwin: 0
infinisil
left a comment
infinisil
left a comment
There was a problem hiding this comment.
Would be great to add more context to this, but I was able to figure it out:
Currently broken = true only causes problems.broken to be generated when allowBroken = false. So if allowBroken = true, no problems.broken exists, and so no error is thrown. But if somebody sets problems.broken directly, there's nothing that prevents it from throwing an error with allowBroken = true.
While this fixes the immediate problem, it's not entirely right, because there's also NIXPKGS_ALLOW_BROKEN and allowBrokenPredicate. Instead the right fix I believe is to move the entire allowBroken logic from the automated problems.broken to the mkIf conditional.
| @@ -512,10 +512,10 @@ in | |||
| # Put the default value for matchers in here (as in, not as an *actual* mkDefault default value), | |||
| # to force it being merged with any custom values instead of being overridden. | |||
| problems.matchers = [ | |||
| { | |||
| (lib.mkIf (!config.allowBroken) { | |||
| kind = "broken"; | |||
| handler = "error"; | |||
There was a problem hiding this comment.
It's also a question whether we should do
| handler = "error"; | |
| handler = if (config.allowBroken ...) then "warn" else "error"; |
or
| handler = "error"; | |
| handler = if (config.allowBroken ...) then "ignore" else "error"; |
Traditionally at least it was ignore, but I'd argue we should make it a warn, because allowBroken is such a heavy-handed toggle, while users now also have the option to individually set problems.handlers.<somePackage>.broken = "ignore".
Sigmanificient
changed the title
allowBroken actully allow evrything marked as brokenallowBroken actully allow everything marked as broken
|
Im currently not seeing how it would be possible to migrate |
| allowBrokenPredicate = | ||
| lib.warnIf (lib.oldestSupportedReleaseIsAtLeast 2605) | ||
| "config.allowBrokenPredicate is deprecated, use config.problems.handlers.myPackage.broken = \"warn\" for individual packages instead." | ||
| config.allowBrokenPredicate; | ||
| in | ||
| if allowBroken then |
There was a problem hiding this comment.
This was on purpose to allow not checking for brokenness when allowBroken is true. Is there a way to get this back?
There was a problem hiding this comment.
Should that not already be the case when the handler is set to ignore? If not i would be very concerned about adding any new automaticProblems to the list.
There was a problem hiding this comment.
It used to not do that until I fixed it in #519600 - but yes, you're right. Just confirmed myself.
3 participants
Noticed this while working on #533355
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.