emacs: teach elisp builders the finalAttrs pattern by jian-lin · Pull Request #330589 · NixOS/nixpkgs

jian-lin · 2024-07-28T06:33:19Z

Description of changes

~~This PR causes 0 rebuilds.~~

Review advice

It is easier to review this PR commit by commit.

The first four commits are small fixes and cleaning. Almost all work is done in the last commit.

Performance overhead of eval time

package set	before	after	changed
emacs.pkgs.elpaPackages	1.925	1.935	+0.35%
emacs.pkgs.melpaPackages	8.312	8.558	+3.0%

The commands used here are

nix nixpkgs#hyperfine -- --warmup 2 --runs 10 'NIXPKGS_ALLOW_BROKEN=1 nix eval --include nixpkgs=$PWD --file . emacs.pkgs.melpaPackages --apply \'pkgSet: map (drv: drv.drvPath) (builtins.filter (p: p.type or null == "derivation") (builtins.attrValues pkgSet))\' --no-eval-cache >/dev/null'

nix nixpkgs#hyperfine -- --warmup 10 --runs 30 'NIXPKGS_ALLOW_BROKEN=1 nix eval --include nixpkgs=$PWD --file . emacs.pkgs.elpaPackages --apply \'pkgSet: map (drv: drv.drvPath) (builtins.filter (p: p.type or null == "derivation") (builtins.attrValues pkgSet))\' --no-eval-cache >/dev/null'

Things done

Add a 👍 reaction to pull requests you find important.

nixf-tidy-review

❌ | Hi! nixf-tidy code linter found some issues in your touched files.If you believe I'm reporting false positives, feel free to ping @inclyc

pkgs/applications/editors/emacs/build-support/elpa.nix

pkgs/applications/editors/emacs/build-support/generic.nix

pkgs/applications/editors/emacs/build-support/trivial.nix

jian-lin · 2024-07-28T06:56:47Z

Hey @Aleksanaa @inclyc, nixf-tidy reports false positives here.

Unused parameters are totally valid in the "build helpers" (or builders) use case, especially combined with the finalAttrs pattern. These unused parameters serve as the interface and documentation of the build helper, providing information about the expected parameters to both the Nix interpreter and people reading the Nix expression:

nix-repl> lib.functionArgs emacs.pkgs.melpaBuild
{ buildInputs = true; commit = true; ename = true; files = true; meta = true; nativeBuildInputs = true; packageRequires = true; pname = false; postInstall = true; postUnpack = true; preUnpack = true; propagatedBuildInputs = true; propagatedUserEnvPkgs = true; recipe = true; version = false; }

Currently, there are other PRs to teach build helpers the finalAttrs pattern, such as go, python and rust. There is also a general PR to ease this transition. Presumably there will be more of these false positives reported in the future.

pkgs/applications/editors/emacs/build-support/elpa.nix

pkgs/applications/editors/emacs/build-support/melpa.nix

jian-lin · 2024-07-28T07:28:16Z

My local run of nixpkgs-review tells me it is indeed 0 rebuilds 🎉

The 1 rebuild detected by ofborg is not relevant.

x86_64-linux nixpkgs-manual

nixos-discourse · 2024-07-28T07:34:17Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/we-added-a-linter-to-nixpkgs-checking-workflows/49722/16

jian-lin · 2024-07-28T07:43:23Z

Actually, those variables in generic.nix are not "unused". They are passed to stdenv.mkDerivation and are used there. It is indeed a hard thing for a parser to understand. 😅

inclyc · 2024-07-28T12:25:02Z

Actually, those variables in generic.nix are not "unused". They are passed to stdenv.mkDerivation and are used there. 😅

I just kindly ask how they are passed to stdenv.mkDerivation ? Maybe we can explicitly use it in the attrset like { inherit files; } ?

It is indeed a hard thing for a parser to understand.

The parser can only understand the variable being used directly by other variables but not do heavily value tracking about how the names are used. There are indeed some cases where function formals (a.k.a parameters) in nix language they are used in builtins.functionArgs, in such case we can ignore the warnings emitted.

These unused parameters serve as the interface and documentation of the build helper, providing information about the expected parameters to both and the Nix interpreter and people reading the Nix expression

I don't think they do exactly provide "expected parameters" to Nix interpreter in this case, because we can remove it without having any side-effect. If they are the part of documentation why shouldn't they go under /** */ as per RFC 0415?

pkgs/applications/editors/emacs/build-support/melpa.nix

ShamrockLee · 2024-07-28T13:08:07Z

Other "truly unused" arguments are debatable. Should we keep them for documentation purposes or remove them to eliminate dead codes?

I agree with @inclyc that we should move them into the documentation. #308377 shows how such unused parts of set-pattern arguments could confuse.

inclyc · 2024-07-28T13:30:38Z

Indeed yes because if declaring a formal arg without actually using it (i.e. by referencing in @arg) may results in a common mistake:

nix-repl> ({ a ? "foo" }@attrs: attrs.a) { }
error: attribute 'a' missing

       at «string»:1:23:

            1| ({ a ? "foo" }@attrs: attrs.a) { }
             |                       ^

nix-repl>

In this case, while the lambda is called, a -> "foo", however attrs -> { }, and thus attrs.a is undefined.

So in our "builder" example, I suggest always explicitly USE the declaration like:

{ foo, bar, ... }@args: someOtherFunction (args // { inherit foo bar; })

jian-lin · 2024-07-28T13:41:41Z

Indeed yes because if declaring a formal arg without actually using it (i.e. by referencing in @arg) may results in a common mistake:
nix-repl> ({ a ? "foo" }@attrs: attrs.a) { }
error: attribute 'a' missing

       at «string»:1:23:

            1| ({ a ? "foo" }@attrs: attrs.a) { }
             |                       ^

nix-repl> 
In this case, while the lambda is called, a -> "foo", however attrs -> { }, and thus attrs.a is undefined.

I know this example a few days ago.

However, this example is not relevant here. There is not any args.a or attrs.a in this PR. It is finalAttrs.a.

So in our "builder" example, I suggest always explicitly USE the declaration like:

{ foo, bar, ... }@args: someOtherFunction (args // { inherit foo bar; })

inclyc · 2024-07-28T13:57:25Z

I mean why don't we need to inherit files here:

https://github.com/NixOS/nixpkgs/blob/7a34d0e63a1ef81af449c9d1cd7aaa583470aa5f/pkgs/applications/editors/emacs/build-support/melpa.nix#L82

like

- inherit packageBuild commit ename recipe; 
+ inherit packageBuild commit ename recipe files;

Aleksanaa · 2024-07-28T13:58:55Z

Then the unused error will move to there?

inclyc · 2024-07-28T14:00:07Z

Then the unused error will move to there?

I guess not, because inherit does not make new definition?

jian-lin · 2024-07-28T14:00:24Z

Adding a unneeded inherit file; just to please the CI? I do not think it is a good idea.

inclyc · 2024-07-28T14:02:56Z

Adding a unneeded inherit file; just to please the CI? I do not think it is a good idea.

Have you tried .overrideing files?

I just cannot understand why finalAttrs contains files attribute if it is not inherited like other names.

jian-lin · 2024-07-28T14:08:44Z

Have you tried .overrideing files?

Parameters added by lib.extendMkDerivation is expected to be overridden with overrideAttrs (not override) likes this

nix-repl> (emacs.pkgs.git-undo.overrideAttrs (old: { files = "foooooo"; })).files
"foooooo"

I just cannot understand why finalAttrs contains files attribute if it is not inherited like other names.

Please have a look at lib.adaptMkDerivation and lib.extendMkDerivation.

jian-lin · 2024-07-28T14:18:01Z

Sorry. I somehow missed this.

Actually, those variables in generic.nix are not "unused". They are passed to stdenv.mkDerivation and are used there. 😅

I just kindly ask how they are passed to stdenv.mkDerivation ?

In generic.nix, pname and version is implicitly passed to stdenv.mkDerivation with the help of lib.adaptMkDerivation. Note // removeAttrs args handledArgs. Please have a look at lib.adaptMkDerivation.

Maybe we can explicitly use it in the attrset like { inherit files; } ?

Note that files is not in generic.nix.

It is indeed a hard thing for a parser to understand.

The parser can only understand the variable being used directly by other variables but not do heavily value tracking about how the names are used. There are indeed some cases where function formals (a.k.a parameters) in nix language they are used in builtins.functionArgs, in such case we can ignore the warnings emitted.

These unused parameters serve as the interface and documentation of the build helper, providing information about the expected parameters to both and the Nix interpreter and people reading the Nix expression

I don't think they do exactly provide "expected parameters" to Nix interpreter in this case, because we can remove it without having any side-effect.

By "providing information about the expected parameters to both the Nix interpreter", I mean you can check lib.functionArgs emacs.pkgs.melpaBuild in the repl. I already gave an example there.

If they are the part of documentation why shouldn't they go under /** */ as per RFC 0415?

Someone has to write the doc string. Someone also has to make the doc system render these nix files. I may do these things in the future, but not now.

inclyc · 2024-07-28T14:22:24Z

Please have a look at lib.adaptMkDerivation.

CC @ShamrockLee who authored those functions. WDYT?

ShamrockLee · 2024-07-28T15:09:39Z

In generic.nix, pname and version is implicitly passed to stdenv.mkDerivation with the help of lib.adaptMkDerivation. Note // removeAttrs args handledArgs. Please have a look at lib.adaptMkDerivation.
CC @ShamrockLee, who authored those functions. WDYT?

lib.adaptMkDerivation is designed not to pass arguments implicitly to the base build helper, in contrast to lib.extendMkDerivations. Instead, arguments are passed explicitly by // removeAttrs args handledArgs.

(People are already doing this a lot in existing build helper definitions, so we still need lib.adaptMkDerivation in addition to lib.extendBuildHelper.)

ShamrockLee · 2024-07-28T15:12:59Z

IMHO, this issue is not specific to lib.adaptMkDerivation and lib.extendMkDerivation. People are already experiencing this in the current way of defining build helpers. An existing example can be found in the description of #308377.

The question is, should we keep syntactically unused arguments in the set pattern of a build helper? Should the set pattern and __functionArgs be considered a form of documentation?

CC @fricklerhandwerk, who devoted a lot of time to the documentation. What do you think about it?

jian-lin · 2024-07-29T01:27:38Z

I add data about performance overhead here.

Before nix-repl> (emacs.pkgs.magit.overrideAttrs (old: { pname = old.pname + "-patched"; })).name "emacs-magit-20240522.204" After nix-repl> (emacs.pkgs.magit.overrideAttrs (old: { pname = old.pname + "-patched"; })).name "emacs-magit-patched-20240522.204"

The default value of doCheck is false.

Without this patch, if there is propagatedBuildInputs in the arguments of genericBuild, it will override the value set by genericBuild. With this patch applied, the argument and the value set by genericBuild are merged instead.

When Emacs starts, package-activate-all finds autoload files and loads them. However, the autoload file generated by trivialBuild is never picked up by package-activate-all. In other words, this feature never works. So let's remove it.

This commit causes 0 rebuilds. The performance overhead of eval time is as follows: | package set | before | after | changed | |--------------------------|--------|-------|---------| | emacs.pkgs.elpaPackages | 1.925 | 1.935 | +0.35% | | emacs.pkgs.melpaPackages | 8.312 | 8.558 | +3.0% | The commands used here are nix nixpkgs#hyperfine -- --warmup 2 --runs 10 'NIXPKGS_ALLOW_BROKEN=1 nix eval --include nixpkgs=$PWD --file . emacs.pkgs.melpaPackages --apply \'pkgSet: map (drv: drv.drvPath) (builtins.filter (p: p.type or null == "derivation") (builtins.attrValues pkgSet))\' --no-eval-cache >/dev/null' nix nixpkgs#hyperfine -- --warmup 10 --runs 30 'NIXPKGS_ALLOW_BROKEN=1 nix eval --include nixpkgs=$PWD --file . emacs.pkgs.elpaPackages --apply \'pkgSet: map (drv: drv.drvPath) (builtins.filter (p: p.type or null == "derivation") (builtins.attrValues pkgSet))\' --no-eval-cache >/dev/null'

Previously, we vendor PR NixOS#234651 because we want to keep the old behavior of filtering out packageRequires from the arguments we pass to the underling stdenv.mkDerivation. Doing so raises the concern about the complexity of PR NixOS#234651. Considering that passing packageRequires to stdenv.mkDerivation also works well, we stop filtering it out and stop vendoring PR NixOS#234651. Now, this PR only uses the existing interface of stdenv.mkDerivation. Even though the name of the build helper is still extendMkDerivation', it is nothing new and has been used in Nixpkgs, such as php.buildComposerProject[1]. [1]: https://github.com/NixOS/nixpkgs/blob/f3834de3782b82bfc666abf664f946d0e7d1f116/pkgs/build-support/php/builders/v1/build-composer-project.nix#L108

jian-lin · 2024-08-05T00:14:58Z

Conflicts are resolved.

jian-lin · 2024-08-13T07:41:12Z

pkgs/applications/editors/emacs/build-support/generic.nix

@@ -97,4 +97,4 @@ stdenv.mkDerivation (finalAttrs: ({
  '' + postInstall;


Almost all other attrs are overridden by user-defined values. Applying that pattern to postInstall means changing postInstall = '' .... '' + postInstall to postInstall = args.postInstall or '' ... '', which means if users define postInstall, their package never does native compilation. I think this is very bad. However, inconsistence is also not good. WDYT?

Well, well, well...

Searching some packages, I have found that e.g. copyDesktopItems injects itself at postInstallHooks.

postInstallHooks+=(copyDesktopItems) # rest of code

It implies that post installation hooks are "queued" but not destroyed/substituted.
(And it explains why we need to never forget the runHook pre<phase-name>...)

That being said, curiously the consistency is not in substituting pre/post phases, but in queueing them.
So I vote for + instead of or.

Note that there are two kinds of postInstall hooks, one is defined in nix files with the attr postInstall, others are defined as an array called postInstallHooks in setup hooks. For the first kind of hook, it is indeed destroyed/substituted. For the second kind of hook, generally it is queued using preInstallHooks+=(...). Here, we only talk about the first kind of hook.

Another question, which one of the two orders should be used for +, postInstall = '' .... '' + postInstall or postInstall = postInstall + '' .... ''?

jian-lin · 2024-08-13T07:44:36Z

pkgs/applications/editors/emacs/build-support/melpa.nix

-    else version;
+    else finalAttrs.version);

  preUnpack = ''


Almost all other attrs are overridden by user-defined values. Should we apply that pattern to preUnpack for consistency, i.e., changing preUnpack = '' ... '' + preUnpack to preUnpack = args.preUnpack or '' ... ''? The same question applies to postUnpack.

preUnpack suggests the idea we need to some preparation to the source so that it can be unpacked by the unpack phase of the builder we are dealing with. Further, this unpack phase is, at least conceptually, a black box.

Think in something like "oh the unpacker does not like dashes in the filename, let's change it to underscores first!" The preUnpack would change the filename, so that it can be passed to the black box.

So I would follow the same as I said to postInstall: + instead of or.

adisbladis · 2024-08-15T04:47:28Z

pkgs/applications/editors/emacs/build-support/melpa.nix

-, recipe ? (writeText "${pname}-recipe" ''
-    (${ename} :fetcher git :url ""
-              ${lib.optionalString (files != null) ":files ${files}"})
+, recipe ? (writeText "${finalAttrs.pname}-recipe" ''


Not something to address here and now:
Do we really need to use writeText for the recipe? Couldn't it be done inline in the builder instead, skipping the intermediate derivation?

Sounds good. I created #334888 to track this potential improvement.

jian-lin requested a review from adisbladis as a code owner July 28, 2024 06:33

github-actions bot added the 6.topic: emacs Text editor label Jul 28, 2024

jian-lin requested a review from AndersonTorres July 28, 2024 06:33

nixf-tidy-review reviewed Jul 28, 2024

View reviewed changes

inclyc reviewed Jul 28, 2024

View reviewed changes

pkgs/applications/editors/emacs/build-support/elpa.nix Outdated Show resolved Hide resolved

pkgs/applications/editors/emacs/build-support/melpa.nix Outdated Show resolved Hide resolved

jian-lin force-pushed the pr/emacs-builder-finaAttrs branch from 4358e1e to 7a34d0e Compare July 28, 2024 07:10

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jul 28, 2024

Aleksanaa changed the title ~~emacs: teach elisp builders the finalAttrs pattern~~ emacs: teach elisp builders the finalAttrs pattern [skip treewide] Jul 28, 2024

ShamrockLee reviewed Jul 28, 2024

View reviewed changes

pkgs/applications/editors/emacs/build-support/melpa.nix Outdated Show resolved Hide resolved

jian-lin force-pushed the pr/emacs-builder-finaAttrs branch 2 times, most recently from d280e2d to f85026e Compare August 4, 2024 02:44

jian-lin marked this pull request as draft August 4, 2024 03:14

jian-lin changed the base branch from master to staging August 4, 2024 03:16

jian-lin marked this pull request as ready for review August 4, 2024 03:16

AndersonTorres approved these changes Aug 4, 2024

View reviewed changes

jian-lin added 9 commits August 5, 2024 07:53

emacs: remove redundant doCheck from genericBuild

675dcef

The default value of doCheck is false.

emacs: remove unused parameters to make nixf-tidy-review bot happy

5805cf2

emacs: make elpa2nix of elpaBuild consistent with melpaBuild

e64ccec

emacs: inherit files in melpaBuild to make nixf-tidy CI happy

bdd7734

jian-lin force-pushed the pr/emacs-builder-finaAttrs branch from 52fb53f to bdd7734 Compare August 5, 2024 00:12

jian-lin mentioned this pull request Aug 6, 2024

lspce-module: 1.1.0-unstable-2024-07-14 -> 1.1.0-unstable-2024-08-04 #332629

Closed

13 tasks

jian-lin commented Aug 13, 2024

View reviewed changes

jian-lin mentioned this pull request Aug 14, 2024

Tracking issue: trivialBuild should retire in favor of melpaBuild #278925

Open

adisbladis reviewed Aug 15, 2024

View reviewed changes

adisbladis approved these changes Aug 15, 2024

View reviewed changes

jian-lin merged commit 4d80793 into NixOS:staging Aug 15, 2024

jian-lin deleted the pr/emacs-builder-finaAttrs branch August 15, 2024 13:40

This was referenced Aug 15, 2024

Support embedding recipe of MELPA packages when generating the index (recipes-archive-melpa.json) #334888

Open

emacs: make melpaBuild accept recipe content as a string #334955

Merged

9999years mentioned this pull request Jan 14, 2025

finalAttrs everywhere / generic makeOverridable #373867

Closed

		@@ -97,4 +97,4 @@ stdenv.mkDerivation (finalAttrs: ({
		'' + postInstall;

Uh oh!

Conversation

jian-lin commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Review advice

Performance overhead of eval time

Things done

Uh oh!

nixf-tidy-review left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jian-lin commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jian-lin commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nixos-discourse commented Jul 28, 2024

Uh oh!

jian-lin commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inclyc commented Jul 28, 2024

Uh oh!

Uh oh!

ShamrockLee commented Jul 28, 2024

Uh oh!

inclyc commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jian-lin commented Jul 28, 2024

Uh oh!

inclyc commented Jul 28, 2024

Uh oh!

Aleksanaa commented Jul 28, 2024

Uh oh!

inclyc commented Jul 28, 2024

Uh oh!

jian-lin commented Jul 28, 2024

Uh oh!

inclyc commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jian-lin commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jian-lin commented Jul 28, 2024

Uh oh!

inclyc commented Jul 28, 2024

Uh oh!

ShamrockLee commented Jul 28, 2024

Uh oh!

ShamrockLee commented Jul 28, 2024

Uh oh!

jian-lin commented Jul 29, 2024

Uh oh!

jian-lin commented Aug 5, 2024

Uh oh!

jian-lin Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

AndersonTorres Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jian-lin Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jian-lin Aug 14, 2024

jian-lin commented Jul 28, 2024 •

edited

Loading

jian-lin commented Jul 28, 2024 •

edited

Loading

jian-lin commented Jul 28, 2024 •

edited

Loading

jian-lin commented Jul 28, 2024 •

edited

Loading

inclyc commented Jul 28, 2024 •

edited

Loading

inclyc commented Jul 28, 2024 •

edited

Loading

jian-lin commented Jul 28, 2024 •

edited

Loading

AndersonTorres Aug 14, 2024 •

edited

Loading

jian-lin Aug 14, 2024 •

edited

Loading