Prevent some rebuilds for future Nix reformats #326430

infinisil · 2024-07-12T02:24:52Z

Description of changes

This is motivated by #322537, which I'd like to get close to 0 rebuilds, although due to the nature of that PR not formatting files modified by other PRs, me making this PR makes the other PR avoid these files entirely, so this PR doesn't have to be merged before the other one.

This PR either avoids Nix files ending up in the build result (so that changing them won't cause rebuilds), or formats Nix files in advance, so that they don't need to be changed in the treewide reformat.

This is effectively part of #301014

Things done

Built all the changed derivations

This work is sponsored by Antithesis ✨

Add a 👍 reaction to pull requests you find important.

This is a rare case of a Nix file actually ending up in the build result. We reformat this now, causing a rebuild, so that we won't cause a rebuild in the treewide reformatting PR.

Atemu · 2024-07-12T12:03:55Z

or formats Nix files in advance, so that they don't need to be changed in the treewide reformat.

This doesn't strike me as a great solution tbh. It just kicks the bucket down the road and will cause rebuilds again when the formatting rules are changed again in the future.

sternenseemann · 2024-07-12T17:15:52Z

The cabalSdist test can be made to not depend on the formatting of the generated Nix file by using haskell.lib.compose.overrideSrc at the call site to use a filtered source. If this filtered source is also passed correctly to the other derivation that uses it (near the call site of generated.nix), all should be fine.

See #320572, #321246.

This is a rare case of a Nix file actually ending up in the build result. We reformat this now, causing a rebuild, so that we won't cause a rebuild in the treewide reformatting PR.

There's no need to use a Nix file in the path here. By using a different file we won't cause rebuilds when we change the Nix file, in particular also when the Nix file is reformatted.

The source included way more files than it really needed. This commit limits it to exactly those it needs. This also makes sure that no rebuild is necessary when any Nix file changes, in particular useful when we reformat the Nix files.

The generated file sets its own directory as the source, including the generated file itself, which causes rebuilds when that file is reformatted. We can avoid this by overriding the source with a filtered version and using that throughout the tests. See NixOS#320572 for more context

infinisil · 2024-07-12T18:47:40Z

This doesn't strike me as a great solution tbh. It just kicks the bucket down the road and will cause rebuilds again when the formatting rules are changed again in the future.

Speaking for the formatting team, we don't expect to have to make any significant changes to the format in the foreseeable future. The only changes to expect soon are bug fixes, which won't change much. So I don't think we need to worry very much about it :)

@sternenseemann Thanks, done that in the last commit now!

philiptaron

A few curious comments.

philiptaron · 2024-07-13T23:26:55Z

pkgs/applications/editors/vim/plugins/get-plugins.nix

Sigh, this is on my list of #208242 things to go make better.

philiptaron · 2024-07-13T23:28:55Z

pkgs/test/make-binary-wrapper/default.nix

+    source=${lib.fileset.toSource {
+      root = ./.;
+      fileset = lib.fileset.unions [
+        (./. + "/${testname}.cmdline")
+        (./. + "/${testname}.c")
+        (lib.fileset.maybeMissing (./. + "/${testname}.env"))
+      ];
+    }}


This would be clearer if extracted to another binding.

I think this is fine, I don't think much is gained from having an intermediate Nix binding (we already assign it to a bash variable here)

philiptaron · 2024-07-13T23:31:54Z

pkgs/test/haskell/cabalSdist/default.nix

+  src = lib.fileset.toSource {
+    root = ./local;
+    fileset = lib.fileset.unions [
+      ./local/app
+      ./local/CHANGELOG.md
+      ./local/local.cabal
+    ];
+  };
+  # This prevents the source from depending on the formatting of the ./local/generated.nix file
+  localRaw = haskell.lib.compose.overrideSrc {
+    inherit src;
+  } (haskellPackages.callPackage ./local/generated.nix {});


Having the src = ./. line in local/generated.nix is not needed now, right? It can be the same as the src above?

Since that's technically a generated file, I think it's cleaner to override the source from here instead

It's named as such but is in no way generated.

I'm pretty sure it was generated using cabal2nix, e.g. check see here, but I guess that's not very relevant at this point anyways

0x4A6F · 2024-07-15T19:01:36Z

Please also tag @NixOS/nix-formatting for PRs touching that topic.

And fix locations to not break the test. This is a rare case where another change is required after formatting. We do this in a separate commit so that we don't need to do it in the treewide reformatting PR.

roberth

Not happy about the formatting at all, but I don't want to block the process of implementing formatting, because that would be even worse.
👍 from me.

roberth · 2024-07-16T18:36:01Z

pkgs/applications/editors/vim/plugins/get-plugins.nix

+    && lib.hasAttrByPath [
+      "src"
+      "outputHash"
+    ] value;


As a nixpkgs-fmt user this feels ugly.
I wonder if this would be ok? I think it's cleaner to use the built-in syntax anyway.

Suggested change

&& lib.hasAttrByPath [

"src"

"outputHash"

] value;

&& (value?src.outputHash);

Indeed this can be simplified and reformatted to just

hasChecksum = value: value ? src.outputHash;

Relevant motto from the RFC:

Bad code does not deserve good formatting.

😆

roberth · 2024-07-16T18:36:57Z

lib/tests/modules/declaration-positions.nix

-  options.nested.nestedLine30 = lib.mkOption {
-    type = lib.types.int;
-  };
+  options.nested.nestedLine30 = lib.mkOption { type = lib.types.int; };


I like my mkOptions multi-line, consistently.
Will I have to adjust?

Oh this is actually an inconsistency in the formatter that should be fixed, because

options.nested.nestedLine30 = { type = lib.types.int; };

expands to

options.nested.nestedLine30 = { type = lib.types.int; };

The same should be done here.

See also this part of the standard

Edit: NixOS/nixfmt#222

Though this is actually less prevalent in practice than it looks like here, because as soon as you have at least two attribute values, it will also get expanded. And effectively all options in NixOS have both a type and a description, which makes two.

I figured it might not be as bad.

Sometimes I start out with only a description or only a type if it's more "technical" like testing or trying something out.
I do anticipate the other attrs, which is why this stood out to me.

roberth · 2024-07-16T18:38:06Z

pkgs/tools/nix/info/relaxedsandbox.nix

-  ''
+  pkgs = import <nixpkgs> { };
+in
+pkgs.runCommand "diagnostics-sandbox" { __noChroot = true; } ''


Do we really need to collapse this attrset into a single line format?
This will add a bunch of diff noise when new attrset are added, won't it?

I'm not sure if we'll go for that in the end, but with NixOS/nixfmt#222 (which also fixes your other complaint), this file would look like this instead:

let pkgs = import <nixpkgs> { }; in pkgs.runCommand "diagnostics-sandbox" { __noChroot = true; } '' set -x # no cache: ${toString builtins.currentTime} test -d "$(dirname "$out")/../var/nix" touch $out ''

infinisil · 2024-07-17T02:56:34Z

@roberth Even if the formatting isn't perfect, having a standard enforced formatting is better than the current churn. Considering that these cases you highlighted are somewhat rare, I don't think we need to block on this. We can also improve the formatter and then do another reformatting pass later (which will then be much smaller in diff size!). If you have more feedback about the formatting, it would be best to open issues in https://github.com/NixOS/nixfmt instead btw :)

roberth · 2024-07-17T09:13:03Z

Even if the formatting isn't perfect, having a standard enforced formatting is better

As you've read, I agreed with this part.
However, considering that this aspect of the formatter is still subject to change, it seems that you have the priority inversed:

than the current churn.

Unnecessary formatter-induced churn is not ok. If the formatter doesn't behave correctly, or some aspects of it are still in development, don't both users with it.
I'm glad we've found this issue early, so that we can resolve it before bothering others with bad layouts and unnecessary churn.

I don't think we need to block on this.

So I think we should block on this, fix the formatter, and do another pass.

Considering that these cases you highlighted are somewhat rare

I assume you haven't run nixfmt on the whole repo.
There's two of them in this small PR, and I would expect more runCommand calls to be affected, because often it's just a single nativeBuildInputs for simple tests.
But you're right, we should be doing more testing ;)

infinisil · 2024-07-17T10:50:54Z

If the formatter doesn't behave correctly, or some aspects of it are still in development

I'd say we should distinguish between different levels of correctness:

Semantic correctness: That the AST isn't changed and that the formatter is idempotent. We have this verified on all Nixpkgs files since recently (and more recently also with nix-instantiate --parse).
Standard matching: That the formatter adheres to the established standard. This can be very fuzzy in certain edge cases, but as we progressed at establishing the initial standard in [RFC 0166] Nix formatting, take two rfcs#166, @piegamesde did a great job keeping the implementation to match decently closely.
Standard correctness: That the standard doesn't have any inconsistencies in itself. We also tried this during RFC development.

So we have 1 now but probably not 2 or 3. These can arguably never be perfect and take more brainpower to resolve, so it's important to consider the cost/value tradeoff. (And in a twisted way, forcing something imperfect onto people might actually get us more help with improving it :P).

But okay, let's reconsider this and either improve this case or have a better justification for not needing to improve it now.

I assume you haven't run nixfmt on the whole repo.

Complete opposite! We've been automatically running nixfmt on the entirety of Nixpkgs already during development of the RFC to help us decide (e.g. see here), and we're continuing to run it on all PRs that change nixfmt (e.g. see here), and of course there's also the treewide reformatting PR that is being worked on recently 😉

roberth · 2024-07-17T11:20:55Z

I assume you haven't run nixfmt on the whole repo.

Complete opposite!

Ah sorry, I was unclear. I meant to refer to this PR specifically, which I conclude is not tree-wide.

doesn't behave correctly

Oh, that was a bit harsh maybe, out of context at least. I meant to refer to your judgement that this may not be the desired behavior.

Anyway, I think we're on the same page now, and I'd really like to thank you for the effort you're putting into this!

nix-info: Format files with nixfmt

4781c8c

This is a rare case of a Nix file actually ending up in the build result. We reformat this now, causing a rebuild, so that we won't cause a rebuild in the treewide reformatting PR.

infinisil requested a review from philiptaron July 12, 2024 02:24

infinisil requested review from figsoda, sternenseemann, maralorn and ncfavier as code owners July 12, 2024 02:24

github-actions bot added 6.topic: haskell 6.topic: vim labels Jul 12, 2024

This was referenced Jul 12, 2024

Treewide Nix reformat pass 1 [skip treewide] #322537

Draft

[Tracking issue] remove all src = ./. entries that unintentionally include their own .nix files #301014

Open

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Jul 12, 2024

infinisil added 4 commits July 12, 2024 20:44

vimPluginsUpdater: Format files with nixfmt

cdca2ed

This is a rare case of a Nix file actually ending up in the build result. We reformat this now, causing a rebuild, so that we won't cause a rebuild in the treewide reformatting PR.

tests.trivial-builders: Prevent rebuilds when Nix files change

518cea2

There's no need to use a Nix file in the path here. By using a different file we won't cause rebuilds when we change the Nix file, in particular also when the Nix file is reformatted.

infinisil force-pushed the formatting-rebuild-fixes branch from bd8dae7 to c05dd3a Compare July 12, 2024 18:44

philiptaron reviewed Jul 13, 2024

View reviewed changes

lib/tests: Format a file with nixfmt

93f2a29

And fix locations to not break the test. This is a rare case where another change is required after formatting. We do this in a separate commit so that we don't need to do it in the treewide reformatting PR.

infinisil requested a review from roberth as a code owner July 16, 2024 17:10

github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Jul 16, 2024

roberth approved these changes Jul 16, 2024

View reviewed changes

wegank added the 12.approvals: 1 label Jul 17, 2024

infinisil mentioned this pull request Jul 17, 2024

Always expand attrs NixOS/nixfmt#222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent some rebuilds for future Nix reformats #326430

Prevent some rebuilds for future Nix reformats #326430

infinisil commented Jul 12, 2024 •

edited

Loading

Atemu commented Jul 12, 2024

sternenseemann commented Jul 12, 2024

infinisil commented Jul 12, 2024

philiptaron left a comment

philiptaron Jul 13, 2024

philiptaron Jul 13, 2024

infinisil Jul 16, 2024

philiptaron Jul 13, 2024

infinisil Jul 16, 2024

philiptaron Jul 16, 2024

infinisil Jul 16, 2024

0x4A6F commented Jul 15, 2024

roberth left a comment

roberth Jul 16, 2024

infinisil Jul 17, 2024

roberth Jul 16, 2024

infinisil Jul 17, 2024 •

edited

Loading

infinisil Jul 17, 2024 •

edited

Loading

roberth Jul 17, 2024

roberth Jul 16, 2024

infinisil Jul 17, 2024

infinisil commented Jul 17, 2024

roberth commented Jul 17, 2024

infinisil commented Jul 17, 2024

roberth commented Jul 17, 2024

Prevent some rebuilds for future Nix reformats #326430

Are you sure you want to change the base?

Prevent some rebuilds for future Nix reformats #326430

Conversation

infinisil commented Jul 12, 2024 • edited Loading

Description of changes

Things done

Atemu commented Jul 12, 2024

sternenseemann commented Jul 12, 2024

infinisil commented Jul 12, 2024

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x4A6F commented Jul 15, 2024

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

infinisil Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Jul 17, 2024

roberth commented Jul 17, 2024

infinisil commented Jul 17, 2024

roberth commented Jul 17, 2024

infinisil commented Jul 12, 2024 •

edited

Loading

infinisil Jul 17, 2024 •

edited

Loading

infinisil Jul 17, 2024 •

edited

Loading