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

melpa2nix: update to work with Emacs HEAD #276943

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

purcell
Copy link
Member

@purcell purcell commented Dec 26, 2023

Description of changes

When building recent Emacs HEAD with Nix, melpaPackages do not build, since the package-build.el version used here relies on a function in package.el that has been removed.

To resolve this, this PR makes melpa2nix use a newer version of package-build that has been fixed to work with Emacs HEAD. See melpa/package-build#87

Some changes have also been necessary to the corresponding patch and to the elisp in melpa2nix.el.

Edited for clarity

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.05 Release Notes (or backporting 23.05 and 23.11 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 Dec 26, 2023
We now use a newer version of package-build, since
previously-necessary functions have been moved/removed from package.el
Emacs 30. See melpa/package-build#87

Consequently, some changes are necessary to the corresponding patch
and to melpa2nix.el, which this commit also contains.
@purcell
Copy link
Member Author

purcell commented Dec 26, 2023

Running nixpkgs-review...

@purcell
Copy link
Member Author

purcell commented Dec 27, 2023

Seems to rebuild all the emacs packages okay on my aarch64-darwin machine, with the exception of those few with failing native portions (like elisp-ffi) which would not have been affected by this change.

Here's an example of the problem this fixes in the wild btw: https://github.com/purcell/nix-emacs-ci/actions/runs/7328520844/job/19956927982

Copy link
Contributor

Successfully created backport PR for release-23.11:

@mweinelt
Copy link
Member

22k rebuilds across four platforms onto master is not really okay. Even if the builds are small, they clog hydra for a long time, disrupting its capability to deliver timely build results to end users.

The rebuild flags really do reflect a traffic signal, and red means you have to stop.

@purcell
Copy link
Member Author

purcell commented Dec 28, 2023

Apologies, I've only opened PRs against master previously, but I think I understand the issue.

Copy link
Contributor

Successfully created backport PR for staging-23.11:

Copy link
Contributor

Git push to origin failed for release-23.11 with exitcode 1

@AndersonTorres
Copy link
Member

Apologies, I've only opened PRs against master previously, but I think I understand the issue.

Don't feel so bad. This was a recent unwritten decision.

@delroth
Copy link
Contributor

delroth commented Dec 29, 2023

Apologies, I've only opened PRs against master previously, but I think I understand the issue.

Don't feel so bad. This was a recent unwritten decision.

CONTRIBUTING.md linked in every single PR says: https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-causing-mass-rebuilds

Furthermore, if the change causes a mass rebuild, use the appropriate staging branch instead

As a rule of thumb, if the number of rebuilds is over 500, it can be considered a mass rebuild.

There is nothing recent about this, it's been the policy for years. While I also don't blame @purcell for this (they didn't press the merge button, and trying to catch these issues is why we do reviews), it's surprising to me that an ex-committer to nixpkgs would claim this is a "recent unwritten decision".

@purcell
Copy link
Member Author

purcell commented Dec 29, 2023

Thanks for sharing the docs. I did find that info after the fact.

I wonder if there's some stricter automation that could prevent this sort of issue, e.g.

  • Make ofborg add a more explicit "merge-prohibited" label that's hard to miss (bright red is not perceived the same by everyone)
  • Produce a failing Actions check on the basis of the target branch and projected number of rebuilds
  • Branch protection rules tied to one of the above, perhaps?

Effort spent on that automation might yield diminishing returns if the problems they mitigate are infrequent, I guess.

@vcunat
Copy link
Member

vcunat commented Dec 29, 2023

"recent": for some period of time, emacs rebuilds were being merged directly into master with the argument that each of the many builds is very cheap.

@adisbladis
Copy link
Member

I could have sworn I checked that this was targeted against staging before merging. That was a mistake.

vcunat added a commit that referenced this pull request Dec 29, 2023
This reverts commit 7b623c3 (PR #277535),
reversing changes made to 0aa7301.
@vcunat
Copy link
Member

vcunat commented Dec 29, 2023

Moved to staging-next branch right now, as a compromise.

@AndersonTorres
Copy link
Member

AndersonTorres commented Dec 29, 2023

Apologies, I've only opened PRs against master previously, but I think I understand the issue.

Don't feel so bad. This was a recent unwritten decision.

CONTRIBUTING.md linked in every single PR says: https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-causing-mass-rebuilds

Furthermore, if the change causes a mass rebuild, use the appropriate staging branch instead

Usually, mass rebuilds were not about the raw counting of packages being rebuilt, but about the depth of dependency tree/graph. Yes, the most glaring effect is the red circles Github bots provide, but usually they appeared for some special packages that were situated deeper in the Nixpkgs build graph.

Softwares like Meson, Cmake or even GCC are indirect dependencies of a huge amount of different and otherwise unrelated packages. It is understandable that their mass rebuilds should be treated more carefully, since they bring profound modifications on the system as a whole.
Further, because of their deep location in this build graph, they are a bottleneck. Providing more and better machines won't reduce the build times a lot, since the bottleneck can't be built in parallel with its dependencies.

The same can't be said of Emacs. Emacs is mostly a dependency of Emacs Lisp packages, and Emacs Lisp packages are small and not useful outside Emacs itself. After Emacs being built, the Elisp packages can ideally be built in parallel.

Further, we have such a huge amount of Elisp packages because their inclusion into Nixpkgs is automated. Very few of them are handwritten. Indeed we indulge ouserlves in relying on Emacs Overlay Project to provide up-to-date versions of those Emacs Lisp packages.

Also, Emacs is not a "plumbing" package. Usually no one except developers care about if their Cmake or Meson packages are up-to-date. The same can't be said about Emacs since it is a package the users interact daily.

Because of all of those things, it was considered OK to merge Emacs changes over master.

There is nothing recent about this, it's been the policy for years.

The unwritten policy of merging Emacs changes over master is almost as old as, well, adisbladis contributions I suppose?

For a long amount of time, before the decision of providing precompiled Elisp packages, Emacs updates were sent to master branch. This trend continued for a very long time after that decision - indeed, I myself refactored the whole expression that builds two mainline, one experimental and two Macport releases, and absolutely no one thrown the Contributing Guide complaining about mass rebuilds.

it's surprising to me that an ex-committer to nixpkgs would claim this is a "recent unwritten decision".

Maybe because you wasn't there?

#190706 - first time I catched Elisp updates triggering red flags
#233301 - cleanup
#235859 - cleanup round 2
#237125 - elisp packages from elpa-devel
#251472 - small cleanup

And not a single mention of "what is a mass rebuild?" from Contributing Guide. Surprising!

P.S.: since you bring my commit bit status, I lost it because I recklessly merged a mass-rebuild modification into master.

Do you think each of the modifications I did above deserves the same exact chastisement?
(Of course I don't expect an answer.)

@purcell
Copy link
Member Author

purcell commented Jan 12, 2024

The commit where this was re-applied in staging-next is 895cd4b, btw. Hoping it lands soon... I couldn't find any way to judge progress or timelines.

@vcunat
Copy link
Member

vcunat commented Jan 12, 2024

It should get into channels over the weekend.

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

Successfully merging this pull request may close these issues.

8 participants