Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libjaylink: Move package definition into by-name directory #338675

Merged
merged 3 commits into from
Sep 7, 2024

Conversation

felixsinger
Copy link
Member

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 31, 2024
@eclairevoyant
Copy link
Contributor

It's discouraged to send a PR just to move a package to by-name since that will be done automatically, though you're welcome to do it if you're fixing other stuff in the package as well (I see for example description has some issues, we can switch to SRI hash, formatting could be updated, ...)

@felixsinger
Copy link
Member Author

It's discouraged to send a PR just to move a package to by-name since that will be done automatically, though you're welcome to do it if you're fixing other stuff in the package as well (I see for example description has some issues, we can switch to SRI hash, formatting could be updated, ...)

Could you explain how it is done automatically? It wasn't done until now.

@eclairevoyant
Copy link
Contributor

It's pending a future PR.

@felixsinger
Copy link
Member Author

felixsinger commented Sep 6, 2024

It's pending a future PR.

So it's not done automatically ;)

It seems integrating the package into the by-name structure does not just have superficial effects, but actual advantages, like a bot that can help with getting patches in. However, I've added more commits for your suggestions. I would appreciate it if we could get this in now.

@eclairevoyant
Copy link
Contributor

It seems integrating the package into the by-name structure does not just have superficial effects

See https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#manual-migration-guidelines.
The reasoning is that it is superficial and simply causes PR spam if everyone moves their packages one-by-one with no other substantial changes, same goes for reformatting. We had this churn with meta.mainProgram, let's not repeat that here.

As there's no substantial change that wouldn't already be covered by a treewide, I'd vote to close this PR.

@felixsinger felixsinger closed this Sep 6, 2024
@felixsinger felixsinger deleted the pkgs/libjaylink/move branch September 6, 2024 22:11
@mweinelt
Copy link
Member

mweinelt commented Sep 6, 2024

It's discouraged to send a PR just to move a package to by-name since that will be done automatically

When? The author is a maintainer and wants to use the merge-bot to get his packages updated. What is your recommended course of action? Waiting for some arbitrary unnamed date in the future seems silly.

@felixsinger felixsinger restored the pkgs/libjaylink/move branch September 6, 2024 22:31
@felixsinger felixsinger reopened this Sep 6, 2024
@felixsinger
Copy link
Member Author

felixsinger commented Sep 6, 2024

@mweinelt Thanks for confirming the merge bot is in use. I've reopened the pull request and also #338674. Hoping to get this in now.

@Atemu
Copy link
Member

Atemu commented Sep 7, 2024

I concur with @mweinelt.

I too dislike moving stuff to by-name (or do nixfmt) just for the sake of it but, in this case, the ultimate intention is to unlock usage of the merge bot; it's almost like an opt-in.

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Sep 7, 2024

It's discouraged to send a PR just to move a package to by-name since that will be done automatically

When? The author is a maintainer and wants to use the merge-bot to get his packages updated. What is your recommended course of action? Waiting for some arbitrary unnamed date in the future seems silly.

It's literally in the contribution guidelines?
The recommended course is make a substantial PR, as I said several times and that is also mentioned in said guidelines linked above.

Again, do we really want these sorts of no-op PRs times tens of thousands of packages?

@Atemu
Copy link
Member

Atemu commented Sep 7, 2024

do we really want these sorts of no-op PRs times tens of thousands of packages?

Nobody is arguing for that extreme here. The intention is clearly to unlock usage of the merge bot in this case. Arguably, that's not even a no-op.

I also highly doubt we even have 10000 individually maintained packages that still have an active maintainer who could conceivably desire to port them.

Even if we had, it's also not like these PRs require significant reviewer attention as they're dead simple. I could have merged a couple of those in the time it took to argue with you about it. I don't see the negative effect.

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Sep 7, 2024

Nobody is arguing for that extreme here. The intention is clearly to unlock usage of the merge bot in this case. Arguably, that's not even a no-op.

It's more of a no-op than mainProgram was, and that was in fact a mess.
And if you're arguing for this PR, then you are in fact arguing that:

  • All PRs that simply move to by-name are valid, which in turn implies that
  • The contribution guidelines don't matter (since no one has discussed PR'ing a change to them), and
  • You're willing to sign up nixpkgs for PR spam (because the same argument would apply to every package, right?)

Or yknow, rather than PR spam, we could actually move on the treewide (#211832)?
(I don't know what's blocking the progress there, so I'd rather discuss there instead.)

@mweinelt
Copy link
Member

mweinelt commented Sep 7, 2024

Stop rule lawyering. We wouldn't have a hint in the bots PRs if it was not supposed to be used. It is fucking exhausting.

Telling us to make progress on the treewide, which is blocked on @infinisil et al is a cop out.

@infinisil
Copy link
Member

I wanted to finish that work, but Things™ happened, and now I'm involved in something completely different and don't have time to finish the automated migration just yet 🙃, sorry for the unexpected delay!

So I think it's totally fair to move packages manually by now, especially since there's the merge bot now that depends on it. I'll submit a PR to update the contributor guidelines, they weren't originally written with all of this in mind.

And if somebody would like to help out with the automated migration, check out NixOS/nixpkgs-vet#56. It's totally doable, but it needs a bit of elbow grease. I'll get to it eventually if nobody else does, though it might still take some time.

@infinisil
Copy link
Member

#340235

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Sep 7, 2024

Stop rule lawyering.

Did you read my other points? My comment was literally the opposite of "rule-lawyering", I explained the spirit of the original guidelines.
I'm still against the PR spam, and personally won't be approving PRs that do this.
Feel free to encourage the spam by merging.

@infinisil
Copy link
Member

@eclairevoyant We need to consider new context as well. The guidelines weren't written with the merge bot in mind, that only came later. With the bot now existing, we should think of such moves to pkgs/by-name not as churn, but rather empowering maintainers to take care of their own packages. You're not just merging such a move, but effectively also merging any future package updates on behalf of the maintainer! I do believe that kind of empowerment is what Nixpkgs is in need.

@0x4A6F 0x4A6F merged commit 442210e into NixOS:master Sep 7, 2024
23 of 25 checks passed
@felixsinger felixsinger deleted the pkgs/libjaylink/move branch September 7, 2024 13:55
@Atemu
Copy link
Member

Atemu commented Sep 7, 2024

It should also be noted that the contributing guidelines are, indeed, guidelines, not rules. Our actual rules are decided through the RFC process and there has been no such RFC banning the manual migration to by-name.

Perhaps we should also make that fact more clear in the document. IMV its purpose is to document the contributor etiquette as we live it (all the "unwritten rules" etc.), not provide hard clear-cut rules on processes and that should be clear to readers too.

@AndersonTorres
Copy link
Member

So I think it's totally fair to move packages manually by now

I am angry and happy rn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants