I think they meant to enforce a rule that people in nixpkgs-committers cannot merge their own PRs to master.
You can establish a policy like that, but not enforce it against willful violations because sockpuppets and newcomers are indistinguishable.
I do like this principle, but I think in nixpkgs this would result in many more stalled PRs.
The risk is also competing for attention with non-committer contributor PRs.
Thanks for the clarification, that point ‘wooshed’ past me.
Yes, but you also misrepresent it in your original post, both by cherry-picking and making incorrect assumptions.
Not sure what to make of this - you ignore the discussion in the PR and then complain that you were the one being ignored?
Why would you expect me to be familiar with the KeePassXC codebase? Never ever have I seen that being expected for any similar change in nixpkgs or elsewhere. I sincerely hope that upstream have a better understanding of the security pertaining to KeePassXC since they’re the ones making it and we’re just packaging it.
No, it’s a potential attack vector because it provides an API for fetching content from the db and usually connects to a browser, which does much more than parsing jpegs and pngs.
This is certainly not true - not everybody using nix knows knows all of its intricacies or even what nix is, since it can for example be used by operations to deploy software on client machines (on macs in particular). We do this at my company and I know of others who do it too. Also, some old passwords may have been imported without updating them, some may be shared and can’t be updated as easily, etc.
Sure, theoretically a successful attack could give arbitrary code execution and allow the attacker to exfiltrate sensitive data, but exactly how easy or feasible do you think such an attack really is? If it’s easy to pull off, that means almost all KeePassXC users are at risk and the feature should likely be removed upstream.
You wrote the following
I agree about HIBP and it’s arguably useful enough to justify inclusion. That said, we need to consider the average keepass user, and even there the average NixOS user, and there’s a decent chance that all passwords are unique and high entropy.
where the first part is positive and I’ve countered the second part. However, I’m mostly referring to jonafato’s comment on HIBP:
I don’t have strong feelings on the default value of this flag. If it’s going to flip, HIBP integration is probably the best reason to do so, as it provides some tangible security benefit to users.
I learned of the HIBP integration a few hours after submitting the PR; not sure why that’s relevant, though.
on 1) if a rule like this existed, you are correct it’s hard to determine if a new person is just a “sockpuppet” and created the account just to bypass the rule, but 1) they would have to be a committer already 2) if/when they are caught, you probably wouldn’t want them to be committers any longer and take their commit bit away. That’s just underhanded terribleness(assuming the rule was in place).
I think version bumps are 99% of the time always safe to merge, so maybe there is an exception here, but even without an exception, it’s a very trivial PR, so it’s easy to see what it’s doing and merge, even if you aren’t the contributor.
Also, just because you can’t commit your own work, as a rule, doesn’t imply that you have to do a thorough code review before committing.
For this particular case, it would at least confirm some other committer thought it was fine to merge as is, despite the comments in the PR request.
I am definitely new to NixOS, and haven’t seen it proposed before, I apologize. It could at least be discouraged that you commit your own PR’s. (which amounts to a rule that’s not enforced via github/etc)
At some point, we will get to a point where we have enough automation to enforce these rules more consistently. There was a RFC to disallow direct pushes to master for example. I think that the desire is there and it’s only a matter of time.
One point that is missing from the discussion (or sorry if I missed it, this thread is getting quite long), is that maintainers of a package should be getting more precedence on decision making. As a maintainer, I want to be able to bump my package and merge it.
It’s not an official rule, but to me, if there is an argument, the maintainer is the one that does most of the work so they get to split the decision.
Guys, what we need here is a process for conflict resolution which we have talked about before in a different context - I’m referencing an earlier comment I made on the point: How many people are paid to work on Nix/Nixpkgs? - #11 by peterhoeg
I’ll come up with an RFC within the next 2 weeks and I hope the participants in this thread will all chime in.
- It might make the PR backlog problem worse, there certainly is a case that it could. But if you pair committers, then it might not.
How can it not make the problem worse if now people need to click merge on more things?
Sure, it can train people to not read what they merge at all, there is a small risk of that.
I am definitely new to NixOS, and haven’t seen it proposed before, I apologize. It could at least be discouraged that you commit your own PR’s. (which amounts to a rule that’s not enforced via github/etc)
You are most surely not at fault, there is too much going on to keep up even without being new. Just wanted to say it is not a topic that goes for long without a discussion.
At some point, we will get to a point where we have enough automation to enforce these rules more consistently. There was a RFC to disallow direct pushes to master for example. I think that the desire is there and it’s only a matter of time.
The pushback is also here, and will not go away anytime soon.
And no-direct-pushes is just about how far the tooling around the CI is from the point where pushing everything through CI is not more annoyance than benefit.
And note that not all of the people who want to make discourage self-merges favour the policies like rapid systematisation and increase in commit access handout, so it’s not like improvements in tooling will automatically solve the remaining arguments.
Sorry to chime in here like that, as this may go a little bit of topic, but I feel you are missing a critical factor here:
Code review “works” at $WORK (henceforth just work) because people, or rather employees, are usually aligned towards common goals or principles, either by their own ambitions or by external forces, like management or customers.
My personal and very limited observation is that these common principles do not exist for nixpkgs or are not really followed up upon by leadership. Unless it comes down to the plumbing of the whole thing, everyone is pretty much free to push in whatever they wish, as long as they find someone to give them a thumbs up on their PR. This will not change simply by enforcing a four eyes principle or through automation. If you want to minimize friction for your PR, you will always be able to find someone who is willing to give you a drive-by approval. More automation will also not change that, as I don’t believe it will address the core problem in any way, it just strives to make the status quo more bearable.
So as long as the people involved in nixpkgs maintenance focus on achieving technical solutions (I count code-review as a technical solution here) for non-technical problems, things will not really improve, they will just appear less problematic for a time until the current “solution” is overwhelmed again.
Feel free do disagree 
So as long as the people involved in nixpkgs maintenance focus on achieving technical solutions (I count code-review as a technical solution here) for non-technical problems, things will not really improve, they will just appear less problematic for a time until the current “solution” is overwhelmed again.
Feel free do disagree
It is even more interesting to look for technical solutions while disagreeing what exactly the problem is, and possibly whether it even exists… Of course, such disagreement might also become a problem, or there can be a disagreement on whether it is.
I have to be honest, I don’t understand this at all. How can there be more total merge button pushes?
The merge button gets clicked the same number of times. Technically there is a touch more work involved, because now Tootie(a committer in this example) needs to say hey Tito(another committer) can you merge my stuff?
Likewise Tito needs to ask Tootie to commit her stuff. But really, if Tito and Tootie generally work on the same sorts of things, they can just pair off each other and Tootie knows to always look through for Tito’s PR’s, and vice versa, instead of just merging their own.
Eventually(and probably very quickly) these pairs will work themselves out.
This goes for ckampka’s note also.
That would not change, see above.
I don’t disagree that the scope changes at an OS level, versus a work product or via OSS vs paid work, But I think the thing here is, even if ckampka is some “10X developer genius type” that is off in the haystack re-ordering esoteric complexities that mere mortals can’t handle, chances are that still shouldn’t get committed until at least one other person understands it, etc.
So while I don’t necessarily disagree with your over-arching thesis, I disagree in practicality. Having another set of eyeballs for code that gets pushed into Nix/NixOS brings more understanding of the various bits and pieces that make up the OS, which overall makes the final product a much better one for everyone involved.
But it’s clearly not something some people seem to think is a very good idea, so we can drop it from discussion and get on with our lives. There is basically no chance that I’ll convince you so the discussion is mostly pointless no?
I never said it would.
For the record, I never claimed any of that.
Also for the record, I never argued against pull request in principle or pull request as a measure to spread knowledge. However, neither is what this thread is about. It is about whether or not we can prevent controversial changes from just getting merged and in my opinion, there is simply no basis for assuming it would. The nixpkgs-committers pool is only growing with a decent chance that at some point you will actually know only a minority of the people involved. Outside of that minority, you have no idea what motivates people to commit to nixpkgs and what their standards look like. Chances are also that if you as someone from your peer group politely (or you simply know someone of IRC who you know shares your goals), they will simply fast-track your code without the controversial discussion even happening. Or, they might just take your side in the argument and just merge anyways.
Code review without alignment does not protect against recklessness, laziness or code getting merged despite disagreements.
Another aspect of this discussion is that the leadership group is relatively amorph. We have one BDFL. After that, some members of the community are more or less established. I am sure that the exact set will vary when asking different people.
I agree that even with all the tooling, there will always be possible venues for conflict to arise. If we had a small group of good leaders, they could also be invoked in those cases to split the decision.
I was also kind of surprised by this merge. I’m not opposed to the change, it’s probably the right decision, but…
AFAICS this case was a bit more subtle though: while someone commented ‘I don’t have strong feelings on the default value of this flag’, and that person was a nixpkgs maintainer, he was not listed as a maintainer for this particular package. Also, he only commented, he chose not to approve the PR.
Since there was some reluctance to the change, I think it would have been reasonable to wait for someone with merge rights to approve the PR before self-merging. As a maintainer without merge rights, I know how frustrating this can be, though 
.
What doesn’t help is that the person who is listed as maintainer for this package seems to be relatively inactive.
Agreed, if this PR was by the package maintainer then I think the merge should’ve been noncontroversial.
I would definatly agree on that. I feel that the NixOS/-pkgs community could really benefit from a more active, maybe Debian-esque leadership system that gives the whole project some sort of focus an direction. That would make things a lot smoother imo.
I would definatly agree on that. I feel that the NixOS/-pkgs community could really benefit from a more active, maybe Debian-esque leadership system that gives the whole project some sort of focus an direction. That would make things a lot smoother imo.
This would be a lot of restructuring. The entire RFC process was painfully constructed to put the judgement calls on larger design decisions onto people who pledge to look into the details of the arguments carefully this specific time on this specific topic, and avoid demanding the same people to read up infinite amount of nuances about too many things.
I don’t think those are contrary, I would argue that those are actually complementary.
Just because you strive for some general alignment, eg. “For 21.04, we want to focus on achiveing XXX” does not mean that you would not need a process to flesh out how to actually achieve this XXX, but it could defiantly help people prioritize where to focus their attention and effort.
Just because you strive for some general alignment, eg. “For 21.04, we want to focus on achiveing XXX” does not mean that you would not need a process to flesh out how to actually achieve this XXX, but it could defiantly help people prioritize where to focus their attention and effort.
Nix* community is historically much better at working together despite being divided on basic priorities than at resolving sometimes literally reverse order of priorities based on vastly differing usecases, preferences and contexts.
This comment was from jonafato and they are a maintainer of the package:
Unless I am overlooking something?
I cannot comment on “historically”, but the current state of nixpkgs suggests more a loss of control and nobody stepping up to take the wheel.