Revert the sudo-rs change: #256294, #253876 #256472
Conversation
This reverts commit 922926c, reversing changes made to 18c2786. As you can see on the PR discussion, this change sparked some controversy, where the original maintainer asked for a full separation of the `sudo` module and `sudo-rs` module, to which, the original PR author refused to comply with. Separations of NixOS modules in contexts of almost drop-in replacements are not new in NixOS, prior art can be found in Forgejo/Gitea, see 248310 for an example. Furthermore, they make sense when an "drop-in replacement" is not totally a drop-in replacement, which is the case as this can be seen in the issue tracker of `sudo-rs`. In the meantime, until we obtain reasonable confidence that `sudo-rs` is 100% a drop-in replacement of `sudo`, it is not reasonable to push the burden of a fused module maintenance on the original maintainer, even if they did not commit anything to that module yet, which is not an argument. Moreover, as this can be seen in the discussion, the original maintainer dropped the ball as they cannot reached a compromise with the original PR author which said they would take care of this. Nevertheless, it does not change that the PR was merged too fast (2 weeks) and caused cross compilation regressions because of ignored comments on `buildPackages` handling which were raised early by the original maintainer. Also, changes to the test driver were done without any look or approval from the code owner of the test driver and without any idea if the change can have bigger consequences. The NixOS 23.11 release process will start in one month and two weeks, this is too close to stabilize such a complicated change, no matter what the original PR author believes it to be. If such change has to pass while keeping the fused NixOS module, it will have to wait after the 23.11 release so we have plenty of time of testing it. Otherwise, this change is free to be sent as another NixOS module as recommended by the original maintainer and other NixOS developers in the discussion.
github-actions
bot
added
6.topic: nixos
|
cc @nbraud as you are not part of the NixOS organization so I cannot add you as a reviewer. |
RaitoBezarius
changed the title
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
|
Honest question: do we have any precedent in NixOS right now for I'll quote FWIW, a grep for (Note that I'm unlikely to go into any deep technical debate here, since TBH my first round of review on the original PR was basically dismissed by the author, and it has now been proven that I actually reported a valid bug… no energy for this.) |
A precedent where it didn't work out and the maintainers of both packages agreed on a split was gitea and forgejo, see #248310 and #244866 (the discussion back then was unfortunately cluttered over a few more PRs/issues, but all of them should be linked in the two I pasted here). |
There was a problem hiding this comment.
Let me just go ahead and approve this, because even though I wasn't invited for a review, reading through the other thread, reverting this is clearly the right move:
For 1) in #253876 (comment)
the actual diff needed to support sudo-rs in the module is really tiny (everything else is refactorings and a couple new options to control behavior that was previously hardcoded)
Not familiar with the details, but this may already be the problem right there: Mixing a feature and a refactoring within a single PR is a recipe for disaster.
@nbraud, if you intend to pursue this further, I'd ask two things of you:
- please try to be more receptive towards feedback and more accommodating in your responses. laying off a PR for a few days when things get too heated, is always an option. finding new maintainers after we burned out existing ones, is not.
- separate new functionality from improving existing code's "poor condition". I know it can be tempting to "just do it", when the code is right there and you're already working on it. However, many people's systems depend on
sudoand so improving that carries vastly greater risks than just adding a mode and it's not fair to expect somebody interested in maintaining usability of the absolute foundation of NixOS to care about the latest rust thing du jour.
for 2) ad separating modules vs growing options: I think #244866 was a lot closer call than this: forgejo is still drop-in compatible and the main reason we separated was small differences in release schedule. Also gitea is hardly what you'd call a "central component for many people's systems".
One thing that I think the Nix community has to continually learn and re-learn is that since our configuration language is a fully functional programming language, there is simply much less need to cram every functionality into the same modules: We have lots of freedom to experiment with almost-compatible-but-not-quite modules, and we can even share the "compatible subset" in a common dependency. All without ever getting into the weeds about whether sudo-rs is or will be fully compatible.
Various factors over the decades (mainly memory being scarce in the first few decades) have contributed to place-oriented-programming becoming the norm and the whole industry is hurting for it. FP does give us the chance to break free from this, but only if we decide to use that freedom. Mindlessly applying patterns that we know from other Distros can only lead us down the same path of brittle rigidity, drama and heartbreak, and if we can avoid that, we'll all be better off.
/rant
There was a problem hiding this comment.
Alongside my response in the other PR (#253876 (comment)) I vouch for
- Not reverting the whole thing without a good reason
- Fixing cross-compile
- Maybe splitting the module (that is a revert of sudo module changes, and addition of sudo-rs)
- Reverting the test-driver change as you see fit, as it doesn't really affect anything other than stderr being visible
|
I've opened a competing PR #256491, which aims to move this forward instead of backward. My resolution is to maintain sudo-rs as the module that does both sudo-rs and sudo and merge those changes into sudo itself PR after PR. And also revert the test driver change. |
|
First a note about something that seems to be quickly becoming a worrying pattern, of which those comments are only the latest incarnation:
I obviously do not know whether that what delroth's objection there, but at least what I understood was that asked for the tests to be separate; when I asked for confirmation on doing so, I was called dismissive and told they were disengaging, but I still separated the testsuites.
It wasn't ignored, I couldn't understand the comment (“where did Overall, if you are arguing that there were significant communication issues, I agree... and even reached out to the moderation team asking whether it would be possible for someone to help mediate, before the PR was merged or anything. However, it takes two to communicate, and I do not think one-sided framing will help resolve the issue. Same goes to @bendlas :
Not sure what you mean here?
Until that point, there was no issue with things being “too heated,” and I indeed let things sit for several days. Some people apparently read that as “being dismissive,” but that was absolutely not the intent. I am neither a native English speaker, nor neurotypical enough to express dismissal this way; if I'm asking someone to clarify, or if I tell them something is a workaround for a bug in NixOS' test driver, I literally only mean that.
Agreed. The interaction in that PR has been a significant burden on me as well (in terms of time and spoons), had a very negative impact on my health both mental and physical, and made me reconsider contributing to nixpkgs at all. I realise you are framing this as concern for delroth, and I am sure it would be easier to find someone to replace me (in terms of what I currently maintain in nixpkgs) but you only seem to see a small part of what's going on here.
I kept the PR's history such that they could be easily separated, and even offered to have it split into 2 PRs. Nobody replied to that at all.
I am aware, and that is why I took so many precautions, documented every possible divergence in behaviour I could find even if it's unlikely to be observable in practice, and actively sought out more reviewers beyond @mkg20001.
Again, I never expected delroth to put in work for sudo-rs support. I would have been entirely find being told “I don't care, just keep our testsuites separate” or such. I do not know why I am being told I think that, but please just stop telling people what their own thoughts are. If there is something that would cause sudo's maintainership more work, just say what the issue is. Lastly, delroth offered to have me take over maintainership of the sudo drv, as a resolution whatever the problem is, and I did reply I would accept that solution if it was the most sensible way forward. Obviously I'd prefer not to pick up sudo's maintainership if I am going to use a different implementation, simply because it would then be more likely issues could creep in that I do not notice (than if I was a developer who uses o.g. sudo) but that is an acceptable solution as far as I'm concerned, especially so if the issue cannot be conveyed across. |
|
So, as far as I can see there are at least 2 ways forward from here:
Obviously, in all case cross compilation needs to be fixed. As I stated before, I am not in favour of (3) because it will be burdensome for users:
As (2) seems currently intractable, as said above I am fine with going on with (1) and accepting responsibility for the |
|
As maintainer of From my reading of @RaitoBezarius's initial comment I suspect this is not the only issue that he feels needs to be resolved though. |
Citing from the original comment:
Emphasis mine. From the very start, delroth only asked about separating everything, then you answered #253876 (comment) <- this was interpreted as being dismissive of the initial request, especially in presence of the clarification from Ma27 ; with known examples (Forgejo/Gitea). Then someone explained it again to you in #253876 (comment)
It is an unfortunate situation, but in general, please consider asking for extra folks who may know about it.
This is completely fine and everyone has the right to not be able to express everything properly, the issue here is that things were steamrolled and now a lot of folks are pulled in to untangle this.
I strongly apologize for this. I think it goes both ways and the biggest problem here, IMHO, is time. No one is really at fault except for being too busy / overworked and focused on a lot of issues at the same time, delroth disengaged because they probably didn't feel in capacity of dealing with this. In those situations, taking the time to say: please take the time to review and let's reach a compromise, is a much much much better way to arrive to a satisfying outcome for everyone.
Apologies for that, I think there are very big miscommunications and understanding issues which does not help anyone's case here. Let's move on to getting out of this.
Let's go for (3), but I don't expect the modules to be unified at any point in time and without express consent from sudo maintainers.
That's a non-concern, sudo and sudo-rs are two different things, at the moment. Users will have to duplicate and that's fine.
That's a non-concern, sudo-rs can disappear if it has to disappear, like sudo can disappear if it has to disappear. We have the knobs to handle what you mentioned, and I prefer working on maintenance sustainability rather than user-friendliness for this type of stuff. Also, I don't think (1) should be regarded as a solution, if anything this is a maintainer throwing the gauntlet and I don't like this and this is not something we want to encourage in the future. Again, moving on. Either, we merge this PR and you can restart from zero whatever you want, or we go with (3) in a reasonable timeline. You have to remember that reverting PRs is not a big deal if they break stuff, you can always resubmit and work on your stuff when it's ready. But if you think of users, I guess you should remember that every minute we spend talking about this and not reverting those changes are minutes where a user will encounter a failed build because this PR regressed actual features. |
|
Look, @nbraud, I fully understand the frustration that comes with having worked on a thing and then running into unexpected roadblocks. I also understand the irony of having waited for reviews only for everybody to come out of the woodwork once it's merged (even though that didn't seem to be the case here). But please understand that this whole issue is not about you personally and any criticism that you may have perceived coming your way is 100% directed just towards what I've seen in the discussions, for the limited time I'm giving myself to look into this. All I can hope for is to help keep up some of the standards that I believe made NixOS what it is today. I have great respect for time invested, but that can never be a substitute for due diligence and correctness, especially in NixOS where nothing stops you from scratching your own itch and publishing on NUR or wherever. And regarding due diligence: 2 weeks may seem like a lot while waiting for reviews and responses, but it's really not, when it comes to something as central to NixOS' functionality, as
Not only. I think @delroth can speak for himself. I have this concern for all of open source. And it's not about who can feasibly replace who. It's more about: Why do we seem to be willing to make people leave over some technical argument? Are we treating each other the same way as the faceless corpo would, that we're trying to build an alternative to? For more thoughts: https://gist.github.com/richhickey/1563cddea1002958f96e7ba9519972d9
With respect, these two approaches seem somewhat contradictory, especially if "accepting responsibility" means "replacing the existing maintainer" in this case. I'd be as happy to welcome you, as the next person. Though please consider first if you're ready to accept that responsibility in full, before doing so. With added responsibilty, the discussions usually become more difficult (and numerous), with even more of those wibbly wobbly hard-to-decide social issues than before. We wouldn't want to burn you out either.
Nobody is telling you what you think. You've been given feedback on what you chose to write out and how it was perceived by readers. @RaitoBezarius said
💯 that. To me it didn't sound like "wow, you're doing good work, would you be willing to accept maintainership", but more like the friendliest possible version of "I give up, just take it from me". And that's really not how I want the conversation to go when the question is: "Will sudo continue to work in the next release?". Or any conversion, for that matter. Again, I respect your hustle, @nbraud, and your willingness to put in work.
IMO the perception of "dismissiveness" comes from the combination of apparently expressing strong opinions about how something ought to be done, in conjunction with not being aware of some of the surrounding issues, be they technical or social. I honestly don't blame you for it (nor would I blame autism). It just takes time to learn the constraints of NixOS, which are different than on other distros. And until then, a certain degree of extra humility can go a long way towards bringing out the good faith in everybody. Btw, I wouldn't call myself neurotypical either, and as it happens, I'm also an Austrian native speaker. Maybe we can meet at an Austrian Nix meetup at some point?
My preferrence: Merge this, then continue with #256491 |
There is another one: locate, plocate and mlocate. |
Indeed, thanks! I think this might be the only other one: looking for Unless people found a creative 3rd way to select based on package name 👀 |
|
Also, I realize @nbraud this may come up as us pinning the blame on you, but to be clear, your PR has no problem per se as long as it is a draft or a work in progress, the big problem which caused this whole mess is the merge decision by @mkg20001 which forces us to examine the whole process. And I think @mkg20001 took the wrong merge decision here. Of course, we don't have the perfect guidelines to let folks decide when to merge or not, but merging is very much a "I take responsibility for merging this", especially when there is controversy. It is by no mean a light decision, and that's why mergers don't go around on a merging spree in general. Once we experience or cause catastrophic issues on master that we run to fix, we learn from that, and we keep in check that everyone tries their best at not reproducing our past mistakes, otherwise, nixpkgs cannot be sustainable. In either case, I think we talked enough about the matter at hand, I remain available on any channel to discuss this further, but I hope this clears out the whole situation and I hope we can restart collaboration on better grounds and let the past be. |
|
Closed as superseded by many other things. |
Description of changes
As you can see on the PR discussion, this change sparked some
controversy, where the original maintainer asked for a full separation
of the
sudomodule andsudo-rsmodule, to which, the original PRauthor refused to comply with.
Separations of NixOS modules in contexts of almost drop-in replacements
are not new in NixOS, prior art can be found in Forgejo/Gitea, see
248310 for an example.
Furthermore, they make sense when an "drop-in replacement" is not
totally a drop-in replacement, which is the case as this can be seen in
the issue tracker of
sudo-rs.In the meantime, until we obtain reasonable confidence that
sudo-rsis100% a drop-in replacement of
sudo, it is not reasonable to push theburden of a fused module maintenance on the original maintainer, even if
they did not commit anything to that module yet, which is not an
argument.
Moreover, as this can be seen in the discussion, the original maintainer
dropped the ball as they cannot reached a compromise with the original
PR author which said they would take care of this.
Nevertheless, it does not change that the PR was merged too fast (2
weeks) and caused cross compilation regressions because of ignored comments on
buildPackageshandling which were raised early by the originalmaintainer. Also, changes to the test driver were done without any look
or approval from the code owner of the test driver and without any idea
if the change can have bigger consequences.
The NixOS 23.11 release process will start in one month and two weeks,
this is too close to stabilize such a complicated change, no matter what
the original PR author believes it to be.
If such change has to pass while keeping the fused NixOS module, it will
have to wait after the 23.11 release so we have plenty of time of
testing it.
Otherwise, this change is free to be sent as another NixOS module as
recommended by the original maintainer and other NixOS developers in the
discussion.
This reverts commit 63ae6fa, reversing
changes made to 922926c.
This is a self-merge hotfix for the sudo-rs tests by mkg2001.
It is actually part of the PR 253876 change and the complete
revert rationale can be found there
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)