check-meta: fix hasNoMaintainers for missing maintainers attr#357674
Closed
asymmetric wants to merge 1 commit intoNixOS:masterfrom
Closed
check-meta: fix hasNoMaintainers for missing maintainers attr#357674asymmetric wants to merge 1 commit intoNixOS:masterfrom
hasNoMaintainers for missing maintainers attr#357674asymmetric wants to merge 1 commit intoNixOS:masterfrom
Conversation
asymmetric
changed the title
hasNoMaintainers for missing maintainers attr
nix-owners
bot
requested review from
Artturin,
Ericson2314,
RossComputerGuy,
emilazy,
philiptaron and
reckenrode
emilazy
reviewed
Nov 20, 2024
Member
emilazy
left a comment
emilazy
left a comment
There was a problem hiding this comment.
SGTM in principle. I assume length (attrs.meta.maintainers or [ ]) == 0 is probably bad for eval perf or something.
This will pick up things like the majority of trivial builders etc. that have no meta, I don’t know if that’s a concern. It might even complain about fetchers? That might be why it’s like that currently.
Member
Could add a check if it's got
Yeah, maybe doing a |
Contributor
Author
@emilazy in my (probably naive) testing with hyperfine, they're ~equivalent in terms of speed:
Where:
|
mweinelt
reviewed
Nov 21, 2024
ofborg
bot
added
10.rebuild-darwin: 0
philiptaron
previously requested changes
Nov 23, 2024
Contributor
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
Requesting changes for simplification.
Previously, when meta.maintainers wasn't defined, hasNoMaintainers would return false, since it relied on its presence to check for its length. Now, it returns true.
asymmetric
force-pushed
the
has-no-maintainer
branch
from
cb115fe to
e4945ac
Compare
Contributor
Author
|
Thanks all for the "code golfing" 😅 In my testing with hyperfine (see above), all solutions were basically equivalent in terms of run time (haven’t tested memory usage though). I still ended up implementing @zimbatm's suggestion, since it's much clearer than my first attempt, and avoids a function call. Still, we haven't addressed the issue of false positives with fetchers & co. @RossComputerGuy could you expand on what you mean by
|
Contributor
Author
|
I'm not sure what the CI failure is about... |
Member
Yes, I think we're changing the previous semantics of 4def222. |
Member
|
Contributor
Author
|
@RossComputerGuy thanks for the pointer (turns out the correct attr was
I believe hooks as well, since they're not FODs. |
Member
|
At which point it would be more helpful to check for version or src to confirm it is a package. |
Contributor
Author
|
Changing the code to hasNoMaintainers = attrs: attrs ? src && attrs.meta.maintainers or [ ] == [ ];and building the
full output |
Member
|
Are we slowly but surely circling back to checking for meta? |
Member
|
I like the idea of weakening |
Contributor
Author
|
Problem is that (at least some) hooks have I don't understand why they do though, given how they're defined: 1 2. |
Member
|
Well, perhaps hooks ought to have maintainers… |
Contributor
Author
|
@emilazy I mean I don't disagree (at all, in fact), but how to go about that? Seems like complexity explosion, from a 1-liner to an fuzzily defined social problem 😅 |
Member
|
It’s an off‐by‐default warning, right? There’s a lot of stuff in the tree with no maintainers already. Warning about more of it only seems bad if it’s stuff that we don’t care about the maintainers for (like fetcher calls); for hooks it seems okay to me personally. |
wegank
added
the
2.status: merge conflict
13 tasks
nixpkgs-ci
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
nixpkgs-ci
bot
added
the
12.approvals: 1
Member
|
fixed by #412184 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, when
meta.maintainerswasn't defined,hasNoMaintainerswould return false, since it relied on its presence to check for its length.Now, it returns true, which seems more logical.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.