-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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: teach elisp builders the finalAttrs pattern #330589
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ | 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
Hey @Aleksanaa @inclyc, Unused parameters are totally valid in the "build helpers" (or builders) use case, especially combined with the 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 |
4358e1e
to
7a34d0e
Compare
My local run of The 1 rebuild detected by ofborg is not relevant.
|
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 |
Actually, those variables in |
I just kindly ask how they are passed to
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
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 |
Indeed yes because if declaring a formal arg without actually using it (i.e. by referencing in
In this case, while the lambda is called, So in our "builder" example, I suggest always explicitly USE the declaration like:
|
I know this example a few days ago. However, this example is not relevant here. There is not any
|
I mean why don't we need to
like - inherit packageBuild commit ename recipe;
+ inherit packageBuild commit ename recipe files; |
Then the unused error will move to there? |
I guess not, because |
Adding a unneeded |
Have you tried I just cannot understand why |
Parameters added by nix-repl> (emacs.pkgs.git-undo.overrideAttrs (old: { files = "foooooo"; })).files
"foooooo"
Please have a look at |
Sorry. I somehow missed this.
In
Note that
By "providing information about the expected parameters to both the Nix interpreter", I mean you can check
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. |
CC @ShamrockLee who authored those functions. WDYT? |
(People are already doing this a lot in existing build helper definitions, so we still need |
IMHO, this issue is not specific to The question is, should we keep syntactically unused arguments in the set pattern of a build helper? Should the set pattern and CC @fricklerhandwerk, who devoted a lot of time to the documentation. What do you think about it? |
I add data about performance overhead here. |
d280e2d
to
f85026e
Compare
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
52fb53f
to
bdd7734
Compare
Conflicts are resolved. |
@@ -97,4 +97,4 @@ stdenv.mkDerivation (finalAttrs: ({ | |||
'' + postInstall; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question, which one of the two orders should be used for +
, postInstall = '' .... '' + postInstall
or postInstall = postInstall + '' .... ''
?
@@ -90,7 +94,7 @@ genericBuild ({ | |||
in | |||
if unstableVersionInNixFormat | |||
then date + "." + time | |||
else version; | |||
else finalAttrs.version); | |||
|
|||
preUnpack = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
, recipe ? (writeText "${pname}-recipe" '' | ||
(${ename} :fetcher git :url "" | ||
${lib.optionalString (files != null) ":files ${files}"}) | ||
, recipe ? (writeText "${finalAttrs.pname}-recipe" '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I created #334888 to track this potential improvement.
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
The commands used here are
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.