treewide: cleanup; move packages to pkgs/by-name; format via nixfmt-rfc-style
#325128
Conversation
NotAShelf
force-pushed
the
treewide-by-name-fmt
branch
from
dbc10e9
to
e7aa63c
Compare
NotAShelf
changed the title
pkgs/by-name; format via nixfmt-rfc-stylepkgs/by-name; format via nixfmt-rfc-style
NotAShelf
force-pushed
the
treewide-by-name-fmt
branch
from
e7aa63c
to
c32d4a8
Compare
NotAShelf
force-pushed
the
treewide-by-name-fmt
branch
from
c1e5ee4
to
139e900
Compare
|
What's the reason for closing this? |
|
I was told in Matrix that such a PR would be discouraged not to step on others' toes. |
|
Weird, that was somehow legit to me. |
|
I am happy to continue with this PR, but I would like to avoid roadblocks down the road stemming from a lack of consensus. If I can get a few more positive responses to this, I'll re-open and proceed. |
|
Can you please ping here the maintainer who advised not to do this? |
|
That'd be @dali99 I believe. |
|
@dali99 Could you please elaborate on this PR ? The only issue I see here is the PR title, besides that, it looked legit to me. |
|
For the right context see and I believe the "stepping on toes" part comes from https://matrix.to/#/!kjdutkOsheZdjqYmqp:nixos.org/$c64jRjBheEYGyhl1XXNsZ-wUGfaahFwRkK3MsIXXG-o?via=lossy.network&via=matrix.org&via=nixos.dev The main points are:
This PR's main motivation seems to be the by-name/formatting. And thus I think these things should not be encouraged yet. Especially not done unilaterally without the real maintainer's involvement. This contributor is new and the advice was more general than necessarily for only this package |
|
You are not wrong that formatting and migrating is my motivation, but its not the main motivation. I am more interested and adding missing licenses, but I'd rather also not leave the packages in their "legacy" state. Some derivations also urgently need maintenance updates, which I'd rather get out of the way while I am already here. |
|
I'm also driven by the will to fix issues when I see them, and I have my own personal rule. Here it is:
Basically, every time I edit a Nix file in a PR, it should be totally valid and following the current best practices at the end of the PR. |
|
Btw, there is no such thing as exclusive ownership in nixpkgs; this is a collaborative project. What being a maintainer entails is that you are making yourself available to fix issues with the package, handle updates, approve PRs, etc. and that you ideally use the package and have some knowledge of whether any change will improve a package or cause a problem with it. This does not preclude making changes to packages you are not a maintainer for; many of our contributions come from non-maintainers. @NotAShelf, please feel free to reopen if you're interested in continuing work on this PR. |
Voila quelqu'un d'eclairevoyant! (french pun intended!) Please forgive me, it's been a while I wanted to do it... |
|
Sure, how should I adjust it? |
|
Since you're modifying only one derivation, I would do: |
|
I plan to move other derivations as well, though. So far I have 7 waiting to be pushed. |
|
Ooh fair enough. Ignore my comment then. |
It was never about exclusive ownership. I'm arguing for respecting each other's responsibilities and following the rules, guidelines, and plans we've agreed on. These are both crucial parts of a collaborative project. I'm just accepting there's nuance around ignoring the plan for the things you're actively responsible for. Misconstruing my sentiment to imply anything else is - how should I say this - Totally uncool.
My own ethos is: |
|
What rule is this PR violating, exactly? |
|
I think formatting files you touch is in the spirit of #322537, which will soon make it mandatory precisely to enable this kind of incremental transition; there’s clearly no question of ownership there and no real grounds to hold back on formatting any given leaf package that doesn’t have a ton of existing open PRs. Moving into If a maintainer objects then of course their reasons should be taken into consideration, and I do think that those migrations should be in separate commits from any other changes to make reviewing the diffs easier. But, while I grant that there is some nuance here, I personally disagree with @dali99 about the extent to which there is any firm consensus or guideline against doing these clean‐ups, as long as you have another meaningful change to make to the packages anyway. The documentation for
…which, while hardly an urgent endorsement, certainly doesn’t advise against it. |
|
ofborg called me to review this, since the package I maintain was touched by it. Let's go:
This should be made on a per package basis, not as part of a treewide PR. |
|
If every minor clean‐up has to be split into one PR per package, people just won’t clean things up. I understood the intent of this PR to be to handle the packages listed as lacking licence information in #43716; I think doing some tidying up along the way is reasonable. I do think there’s a threshold of change where it makes sense to split things up and get individual review but I’m not sure that would reach it. That said, I see that the change currently in the PR is just a mechanical update and clean‐up of a package that isn't lacking licence information; for that kind of thing I agree that it should be either separated out into its own PR or done systematically in one go. |
|
The more people we involve, the more we will have different opinionated overviews. |
|
My personal opinion is that we should have done treewide formats and by-name moves a long time ago. And that at this point the churn generated by not doing this is worse than any merge conflicts we would've had. But still I'm arguing for following the plan.
When we don't have consensus it's more important to be careful. Not having consensus doesn't mean you can do whatever you prefer - it means you should tread lightly. Especially for treewides Social dynamics are complex, this is a place where it can be hard to say no or to object to things. Therefore a large part of the responsibility lies with each of us to try to avoid putting each other in these situations in the first place. What I mean by this more concretely is that you shouldn't put the responsibility on other people to object. Don't make people feel as though they've been ran over just because they're quiet. |
|
I think there is perhaps some confusion here because I believe “treewide:” in the PR is only meant to reflect that it touches multiple packages rather than an intent to touch every single package. But I don’t feel like I’m doing a good job at conveying the nuances of my position here, so I’ll bow out too. |
|
@emilazy I feel you. I originally spent hours yesterday trying to convey my feelings as accurate as possible. Culminating in a very large wall of text and one light mental breakdown about the changing culture in nixpkgs over the last six years (suffice to say I didn't send it). I felt quite hurt by @eclairevoyant s comment. But I'm not good at writing accurately, concisely, and politely. I'm sure we all actually have much more nuanced opinions than what we are managing to express here. @NotAShelf I'm really sorry your PR was turned into this |
NixOS/rfcs#140 (by-name structure) and NixOS/rfcs#166 (nixfmt-rfc-style) getting merged represents net community consensus. There is nothing to tread lightly here about, as this follows the guidelines/plans/whatever as written in this very repo. So I'm not sure why you think there is a lack of consensus here. |
I am not talking about every minor cleanup. I am talking about "maintenance updates". "Maintenance updates" has a large latitude of meaning. I am doing a maintenance update on the SDL ecosystem, since the original maintainers retired. I tried to do maintenance updates to mesa expressions, and a single whitespace after a Maintenance updates are not necessarily minor cleanups.
I agree with you here. |
I have to believe you are being intentionally obtuse here and I will therefore disengage, this is the second time you've managed to carefully misunderstand my entire argument. This is entirely a question of how, when and not if, and has always been this. The fact that we've agreed on that this will happen does not mean we've agreed on it happening this way, or that it will happen now. There was discussion around enabling the formatting lint in for example python. But they can do that because the python team is responsible for the python tree. We obviously disagree as to whether or not this fits the "exception" for by-name migrations. But the main crux of the argument isn't that a package can or can't be moved. It's that you might not the one who should be making that decision. I don't believe this light encouragement trumps giving that agency to our maintainers. You might believe something different. |
|
I'm not misunderstanding anything; you seem to believe that maintainership is a form of ownership of a package and they have the final say on every change that happens on the package and can even block improvements, even if they don't actually cause problems to the packaging. I cannot agree with that, nor have I seen that being put into practice. Especially when we have ratcheting checks that will eventually force the move and reformatting anyway, discouraging/bikeshedding someone that you refer to as a "new contributor" simply doing the sort of PR that has been done hundreds of times already seems unnecessarily hostile and capricious. Though I can see a good argument for breaking this up into multiple PRs instead of a treewide, as suggested above. |
Description of changes
Currently going over #43716 to figure out licenses for packages without one, and performing maintenance (as in cleanup and formatting) on packages before I update and add license in a separate PR.
nixfmt-rfc-style.rec,with liband other bad patterns.This'll probably be large as there are lots of packages to go over.
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.