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

emacs: migrate pkgs/build-support/emacs to pkgs/applications/editors/… #315949

Merged
merged 2 commits into from
Jul 6, 2024
Merged

emacs: migrate pkgs/build-support/emacs to pkgs/applications/editors/… #315949

merged 2 commits into from
Jul 6, 2024

Conversation

AndersonTorres
Copy link
Member

@AndersonTorres AndersonTorres commented May 30, 2024

Waiting #316726
https://nixpk.gs/pr-tracker.html?pr=316726

Description of changes

As a consequence of restrictions imposed by RFC 140 - Simple Package Paths -,
files related to a package should be confined on the package directory.

Certainly this restriction does not apply to packages outside by-name hierarchy.

Nonetheless, this is an interesting organization heuristics: things that affect
Emacs should be confined inside Emacs directory.

Indeed a similar task was done before - namely, the migration of emacs-modes to
elisp-packages: #123859

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.

@github-actions github-actions bot added the 6.topic: emacs Text editor label May 30, 2024
@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 May 30, 2024
@AndersonTorres AndersonTorres marked this pull request as ready for review May 30, 2024 20:49
Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation?

I think pkgs/build-support/emacs is pretty clear. In addition, renaming files makes it harder to use git blame.

@AndersonTorres
Copy link
Member Author

pkgs/by-name requires all files used by a package should be in a same directory.
Yes, I know Emacs cannot be migrated yet, however keeping related things together (instead of scattered around the codebase) conveys a better organization and avoids hopping over otherwise unrelated directory tress.

And moving files (without editing them) don't affect the blame mechanics.
Indeed this blaming issue was bring at the discussions of the by-name massive migration already.

@jian-lin
Copy link
Contributor

pkgs/by-name requires all files used by a package should be in a same directory. Yes, I know Emacs cannot be migrated yet

What else prevent Emacs from migrating to by-name if this PR is merged?

And moving files (without editing them) don't affect the blame mechanics.

Good to know that. TIL: git log --follows can also follow file renaming.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 31, 2024

What else prevent Emacs from migrating to by-name if this PR is merged?

The most obvious is the use of darwin.apple_sdk.callPackage instead of pure callPackage.
This is very hard, given that whole banzé about splicing and other dark corners of Nixpkgs.

The second, and conceptually most complicated to deal with, is the use of recurseIntoAttrs, or, more accurately, the fact we tailored a custom wrapper over mkDerivation to use as a single function that builds all four Emacs versions (namely, 28, 29 and their Macport counterparts) and a convenient bunch of flavors (gtk, pgtk, nox...).

This is a similar situation to some other package sets like the multiple versions of nv-codec-headers.

@jian-lin
Copy link
Contributor

jian-lin commented Jun 2, 2024

I am slightly against this PR because

  • pkgs/build-support/ is a good fit for these files. There are over 400 other "build-support" files like these in that directory.
  • whether Emacs can be migrated to "by-name" is unclear to me, which makes the potential benefit of this PR rather small.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Jun 2, 2024

pkgs/build-support/ is a good fit for these files. There are over 400 other "build-support" files like these in that directory.

Almost 500 files inside less than 200 directories? It looks like a mess if you ask me.
This is why I do not believe the old hierarchy is a good fit for anything at all.

Further,

  • We can easily throw everything at misc and conceptually nothing will change into the old hierarchy.
  • The fetchers can - and certainly will - be migrate to by-name. This will remove most of the raison d'ètre of build-support directory
  • many of the files inside are independent packages themselves. They can be migrated to by-name, defeating even more the purposes of this directory.

Further

  • pkgs/build-support/ was a serious hurdle when I tried to migrate gettext to by-name, because gettext expression tries to access a file outside gettext directory - namely, a setup hook that consisted on Bash files.

Further

  • way before RFC 140 be a thing, emacs-modes floated around pkgs and it was migrated to elisp-packages. The benefits of this migration were rather small initially, nonetheless it certainly improved Emacs Nix ecosystem maintenance along the Nixpkgs life line, even more after that dreadful debasement of Emacs to staging.

whether Emacs can be migrated to "by-name" is unclear to me, which makes the potential benefit of this PR rather small.

The organization provided by this provides a way bigger benefit, I would say, given what I wrote above.

@jian-lin
Copy link
Contributor

jian-lin commented Jun 2, 2024

I am convinced by you.

One way to avoid conflict with changes targeting the staging branch is to wait for another staging cycle before merging this PR. It is OK for you?

  • way before RFC 140 be a thing, emacs-modes floated around pkgs and it was migrated to elisp-packages. The benefits of this migration were rather small initially, nonetheless it certainly improved Emacs Nix ecosystem maintenance along the Nixpkgs life line, even more after that dreadful debasement of Emacs to staging.

I do not follow. That massive rebuild is caused by the changed attributes, such as buildPhase and installPhase, of derivations, which is orthogonal to file locations.

@AndersonTorres
Copy link
Member Author

One way to avoid conflict with changes targeting the staging branch is to wait for another staging cycle before merging this PR. It is OK for you?

Yes, it is OK to me.

I do not follow. That massive rebuild is caused by the changed attributes, such as buildPhase and installPhase, of derivations, which is orthogonal to file locations.

I was not talking about the migration, apologies for not being clear.

I was saying that this mass-rebuild is very localized: while a modification at (say) meson setup hook propagates over all Nixpkgs, the modification above affects only the Elisp ecosystem.
Therefore, it makes sense to confine these files inside Emacs subdir.

@jian-lin
Copy link
Contributor

jian-lin commented Jul 6, 2024

Hi, since the staging-next branch is just merged, it's time to do the last rebase!

@jian-lin jian-lin requested a review from Atemu July 6, 2024 12:36
…d-support

As a consequence of restrictions imposed by RFC 140 - Simple Package Paths [1]
-, files related to a package should be confined on the package directory.

Certainly this restriction does not apply to packages outside by-name hierarchy.

Nonetheless, this is an interesting organization heuristics: things that affect
Emacs should be confined inside Emacs directory. Besides a future migration, the
"debuggability" of a framework is way more enhanced when we know how to find all
its files.

A similar task was done before, when RFC 140 was not a thging yet - namely, the
migration of emacs-modes to elisp-packages [2].

[1] NixOS/rfcs#140
[2] #123859
Because the directories were moved.
@AndersonTorres
Copy link
Member Author

Hi, since the staging-next branch is just merged, it's time to do the last rebase!

Done!

Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to eval some related packages and find no issue.

@AndersonTorres
Copy link
Member Author

This is a similar situation to some other package sets like the multiple versions of nv-codec-headers.

Answering myself, this example is outdated - and it can be a good news!

#324199

@jian-lin jian-lin merged commit 5d5046c into NixOS:master Jul 6, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: emacs Text editor 6.topic: policy discussion 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.

2 participants