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

treewide: migrate packages to pkgs/by-name, take 1 #354531

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

Aleksanaa
Copy link
Member

@Aleksanaa Aleksanaa commented Nov 8, 2024

[RFC 140 2b] pkgs/by-name: Automated migration, take 1

We are migrating packages that meet below requirements:

  1. using callPackage
  2. called path is a directory
  3. overriding set is empty ({ })
  4. not containing path expressions other than relative path (to make nixpkgs-vet happy)
  5. not referenced by nix files outside of the directory, other than pkgs/top-level/all-packages.nix
  6. not referencing nix files outside of the directory
  7. not referencing default.nix (since it's changed to package.nix)

The tool is here: https://github.com/Aleksanaa/by-name-migrate. Try reproduce yourself.

@Aleksanaa Aleksanaa marked this pull request as draft November 8, 2024 16:36
@Aleksanaa Aleksanaa force-pushed the by-name-1 branch 2 times, most recently from 7633039 to 654a4d4 Compare November 8, 2024 17:39
@Aleksanaa
Copy link
Member Author

Aleksanaa commented Nov 8, 2024

Todo:

  • Check whether passthru.updateScript has a path instead of checking files having "update" in the name
  • Try evaluating files moved to pkgs/by-name (probably by nix repl stdin/stdout pipe again)

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 8, 2024
@Aleksanaa Aleksanaa force-pushed the by-name-1 branch 2 times, most recently from aeeb365 to 747dd1c Compare November 9, 2024 11:20
We are migrating packages that meet below requirements:

1. using `callPackage`
2. called path is a directory
3. overriding set is empty (`{ }`)
4. not containing path expressions other than relative path (to
makenixpkgs-vet happy)
5. not referenced by nix files outside of the directory, other
than`pkgs/top-level/all-packages.nix`
6. not referencing nix files outside of the directory
7. not referencing `default.nix` (since it's changed to `package.nix`)
8. `outPath` doesn't change after migration

The tool is here: https://github.com/Aleksanaa/by-name-migrate.
@Aleksanaa
Copy link
Member Author

Ran nixpkgs-review locally:

--------- Impacted packages on 'x86_64-linux' ---------
--------- Report for 'x86_64-linux' ---------

@Aleksanaa Aleksanaa removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@emilazy
Copy link
Member

emilazy commented Nov 9, 2024

Result of nixpkgs-review pr 354531 run on aarch64-linux 1

I consider the criteria and checks used by the script to be adequately conservative. There will be conflicts for open PRs, but that seems unavoidable; we could skip packages that have PRs active in a recent time‐frame but Git is decent at handling renames, so I think that’s probably not necessary, especially since so many PRs enact these moves themselves lately. Let’s get this done 🚀

If there are unforeseen painful consequences, I will try to revert ASAP.

@emilazy emilazy merged commit fa2cae8 into NixOS:master Nov 9, 2024
10 of 13 checks passed
@azuwis
Copy link
Contributor

azuwis commented Nov 9, 2024

Before: 1233k pkgs/top-level/all-packages.nix
After:   645k pkgs/top-level/all-packages.nix

@romildo romildo mentioned this pull request Nov 9, 2024
13 tasks
@trofi
Copy link
Contributor

trofi commented Nov 9, 2024

Any plans to automatically update pending PRs? Or people have to fix the arisen conflicts manually?

@emilazy
Copy link
Member

emilazy commented Nov 10, 2024

I think that we could automatically rebase PRs but it would have consequences (e.g., breaking commit signatures). It seems like a lot of the conflicting PRs already involve a manual move to by-name, which would be trickier to handle (e.g. do we just drop the resulting empty commits? if they include formatting too as some of them do, the resulting commit message would become inaccurate and there’s no good way to automate fixing that)… maybe an approach avoiding touching packages with in‐flight PRs would have been better. OTOH, it would also probably have stretched out the process longer as we keep batching up packages that had fallen inactive over time, and increased the amount of active packages that need someone to manually migrate them.

Not sure if this was the right call or not – it seemed like the migration plans had stalled out for long enough, and the obligatory “move to pkgs/by-name” commits were so ubiquitous, that the time had come to bite the bullet and get it over with, but I’m open to feedback. It’d probably be even more disruptive to revert at this point, though, so it would probably just constitute lessons for next time…

@Aleksanaa
Copy link
Member Author

I don't consider merge conflict as necessary, it only affects people not knowing how to use git. Once you pick up git as your workflow, you have to learn to deal with merge conflicts.

This may have an impact on some older PRs, but if it is an unnecessary functional improvement, "the contributor is no longer there" is basically the same as "there is a pending issue but no response", and is often not what we want to see. For other more critical/valuable PRs, rebasing PR in case contributor is not responding has long been a common practice among committers (and gh pr checkout is also quite handy for this).

I do hope that GitHub can have rebase/fixup tools as web app, so that we will not break signatures and can it be more friendly to people who do not know how to use Git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants