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

Prevent some rebuilds for future Nix reformats #326430

Merged
merged 6 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions lib/tests/modules.sh
Original file line number Diff line number Diff line change
Expand Up @@ -542,21 +542,21 @@ checkConfigOutput '^"pear\\npear"$' config.twice.raw ./merge-module-with-key.nix

# Declaration positions
# Line should be present for direct options
checkConfigOutput '^10$' options.imported.line10.declarationPositions.0.line ./declaration-positions.nix
checkConfigOutput '/declaration-positions.nix"$' options.imported.line10.declarationPositions.0.file ./declaration-positions.nix
checkConfigOutput '^14$' options.imported.line14.declarationPositions.0.line ./declaration-positions.nix
checkConfigOutput '/declaration-positions.nix"$' options.imported.line14.declarationPositions.0.file ./declaration-positions.nix
# Generated options may not have line numbers but they will at least get the
# right file
checkConfigOutput '/declaration-positions.nix"$' options.generated.line18.declarationPositions.0.file ./declaration-positions.nix
checkConfigOutput '^null$' options.generated.line18.declarationPositions.0.line ./declaration-positions.nix
checkConfigOutput '/declaration-positions.nix"$' options.generated.line22.declarationPositions.0.file ./declaration-positions.nix
checkConfigOutput '^null$' options.generated.line22.declarationPositions.0.line ./declaration-positions.nix
# Submodules don't break it
checkConfigOutput '^39$' config.submoduleLine34.submodDeclLine39.0.line ./declaration-positions.nix
checkConfigOutput '/declaration-positions.nix"$' config.submoduleLine34.submodDeclLine39.0.file ./declaration-positions.nix
checkConfigOutput '^45$' config.submoduleLine38.submodDeclLine45.0.line ./declaration-positions.nix
checkConfigOutput '/declaration-positions.nix"$' config.submoduleLine38.submodDeclLine45.0.file ./declaration-positions.nix
# New options under freeform submodules get collected into the parent submodule
# (consistent with .declarations behaviour, but weird; notably appears in system.build)
checkConfigOutput '^34|23$' options.submoduleLine34.declarationPositions.0.line ./declaration-positions.nix
checkConfigOutput '^34|23$' options.submoduleLine34.declarationPositions.1.line ./declaration-positions.nix
checkConfigOutput '^38|27$' options.submoduleLine38.declarationPositions.0.line ./declaration-positions.nix
checkConfigOutput '^38|27$' options.submoduleLine38.declarationPositions.1.line ./declaration-positions.nix
# nested options work
checkConfigOutput '^30$' options.nested.nestedLine30.declarationPositions.0.line ./declaration-positions.nix
checkConfigOutput '^34$' options.nested.nestedLine34.declarationPositions.0.line ./declaration-positions.nix

cat <<EOF
====== module tests ======
Expand Down
30 changes: 19 additions & 11 deletions lib/tests/modules/declaration-positions.nix
Original file line number Diff line number Diff line change
@@ -1,49 +1,57 @@
{ lib, options, ... }:
let discardPositions = lib.mapAttrs (k: v: v);
let
discardPositions = lib.mapAttrs (k: v: v);
in
# unsafeGetAttrPos is unspecified best-effort behavior, so we only want to consider this test on an evaluator that satisfies some basic assumptions about this function.
assert builtins.unsafeGetAttrPos "a" { a = true; } != null;
assert builtins.unsafeGetAttrPos "a" (discardPositions { a = true; }) == null;
assert
builtins.unsafeGetAttrPos "a" (discardPositions {
a = true;
}) == null;
{
imports = [
{
options.imported.line10 = lib.mkOption {
options.imported.line14 = lib.mkOption {
type = lib.types.int;
};

# Simulates various patterns of generating modules such as
# programs.firefox.nativeMessagingHosts.ff2mpv. We don't expect to get
# line numbers for these, but we can fall back on knowing the file.
options.generated = discardPositions {
line18 = lib.mkOption {
line22 = lib.mkOption {
type = lib.types.int;
};
};

options.submoduleLine34.extraOptLine23 = lib.mkOption {
options.submoduleLine38.extraOptLine27 = lib.mkOption {
default = 1;
type = lib.types.int;
};
}
];

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

options.submoduleLine34 = lib.mkOption {
options.submoduleLine38 = lib.mkOption {
default = { };
type = lib.types.submoduleWith {
modules = [
({ options, ... }: {
options.submodDeclLine39 = lib.mkOption { };
})
(
{ options, ... }:
{
options.submodDeclLine45 = lib.mkOption { };
}
)
{ freeformType = with lib.types; lazyAttrsOf (uniq unspecified); }
];
};
};

config = {
submoduleLine34.submodDeclLine39 = (options.submoduleLine34.type.getSubOptions [ ]).submodDeclLine39.declarationPositions;
submoduleLine38.submodDeclLine45 =
(options.submoduleLine38.type.getSubOptions [ ]).submodDeclLine45.declarationPositions;
};
}
33 changes: 21 additions & 12 deletions pkgs/applications/editors/vim/plugins/get-plugins.nix
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
with import <localpkgs> {};
with import <localpkgs> { };
let
inherit (vimUtils.override {inherit vim;}) buildVimPlugin;
inherit (vimUtils.override { inherit vim; }) buildVimPlugin;
inherit (neovimUtils) buildNeovimPlugin;

generated = callPackage <localpkgs/pkgs/applications/editors/vim/plugins/generated.nix> {
inherit buildNeovimPlugin buildVimPlugin;
} {} {};
hasChecksum = value:
lib.isAttrs value && lib.hasAttrByPath ["src" "outputHash"] value;
getChecksum = name: value:
if hasChecksum value then {
submodules = value.src.fetchSubmodules or false;
sha256 = value.src.outputHash;
rev = value.src.rev;
} else null;
} { } { };
hasChecksum =
value:
lib.isAttrs value
&& lib.hasAttrByPath [
"src"
"outputHash"
] value;
Comment on lines +12 to +15
Copy link
Member

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

😆

getChecksum =
name: value:
if hasChecksum value then
{
submodules = value.src.fetchSubmodules or false;
sha256 = value.src.outputHash;
rev = value.src.rev;
}
else
null;
checksums = lib.mapAttrs getChecksum generated;
in
lib.filterAttrs (n: v: v != null) checksums
lib.filterAttrs (n: v: v != null) checksums
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Just some text
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
norefsDup = writeText "hi" "hello";
helloRef = writeText "hi" "hello ${hello}";
helloRefDup = writeText "hi" "hello ${hello}";
path = ./samples.nix;
pathLike.outPath = ./samples.nix;
path = ./apath.txt;
pathLike.outPath = ./apath.txt;
helloFigletRef = writeText "hi" "hello ${hello} ${figlet}";
selfRef = runCommand "self-ref-1" { } "echo $out >$out";
selfRef2 = runCommand "self-ref-2" { } ''echo "${figlet}, $out" >$out'';
Expand Down
19 changes: 15 additions & 4 deletions pkgs/test/haskell/cabalSdist/default.nix
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
{ lib, haskellPackages, runCommand }:
{ lib, haskell, haskellPackages, runCommand }:

let
localRaw = haskellPackages.callPackage ./local/generated.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 {});
Comment on lines +4 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

in
lib.recurseIntoAttrs rec {

Expand All @@ -20,14 +31,14 @@ lib.recurseIntoAttrs rec {
assumptionLocalHasDirectReference = runCommand "localHasDirectReference" {
drvPath = builtins.unsafeDiscardOutputDependency localRaw.drvPath;
} ''
grep ${./local} $drvPath >/dev/null
grep ${src} $drvPath >/dev/null
touch $out
'';

localHasNoDirectReference = runCommand "localHasNoDirectReference" {
drvPath = builtins.unsafeDiscardOutputDependency localFromCabalSdist.drvPath;
} ''
grep -v ${./local} $drvPath >/dev/null
grep -v ${src} $drvPath >/dev/null
touch $out
'';
}
17 changes: 13 additions & 4 deletions pkgs/test/make-binary-wrapper/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,25 @@ let
makeGoldenTest = testname: runCommand "make-binary-wrapper-test-${testname}" env ''
mkdir -p tmp/foo # for the chdir test

params=$(<"${./.}/${testname}.cmdline")
source=${lib.fileset.toSource {
root = ./.;
fileset = lib.fileset.unions [
(./. + "/${testname}.cmdline")
(./. + "/${testname}.c")
(lib.fileset.maybeMissing (./. + "/${testname}.env"))
];
}}
Comment on lines +18 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be clearer if extracted to another binding.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)


params=$(<"$source/${testname}.cmdline")
eval "makeCWrapper /send/me/flags $params" > wrapper.c

diff wrapper.c "${./.}/${testname}.c"
diff wrapper.c "$source/${testname}.c"

if [ -f "${./.}/${testname}.env" ]; then
if [ -f "$source/${testname}.env" ]; then
eval "makeWrapper ${envCheck} wrapped $params"
env -i ./wrapped > env.txt
sed "s#SUBST_ARGV0#${envCheck}#;s#SUBST_CWD#$PWD#" \
"${./.}/${testname}.env" > golden-env.txt
"$source/${testname}.env" > golden-env.txt
if ! diff env.txt golden-env.txt; then
echo "env/argv should be:"
cat golden-env.txt
Expand Down
21 changes: 10 additions & 11 deletions pkgs/tools/nix/info/multiuser.nix
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
let
pkgs = import <nixpkgs> {};
in pkgs.runCommand "diagnostics-multiuser"
{ }
''
set -x
# no cache: ${toString builtins.currentTime}
# For reproducibility, nix always uses nixbld group:
# https://github.com/NixOS/nix/blob/1dd29d7aebae706f3e90a18bbfae727f2ed03c70/src/libstore/build.cc#L1896-L1908
test "$(groups)" == "nixbld"
touch $out
''
pkgs = import <nixpkgs> { };
in
pkgs.runCommand "diagnostics-multiuser" { } ''
set -x
# no cache: ${toString builtins.currentTime}
# For reproducibility, nix always uses nixbld group:
# https://github.com/NixOS/nix/blob/1dd29d7aebae706f3e90a18bbfae727f2ed03c70/src/libstore/build.cc#L1896-L1908
test "$(groups)" == "nixbld"
touch $out
''
5 changes: 3 additions & 2 deletions pkgs/tools/nix/info/relaxedsandbox.nix
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
let
pkgs = import <nixpkgs> {};
in pkgs.runCommand "diagnostics-sandbox"
pkgs = import <nixpkgs> { };
in
pkgs.runCommand "diagnostics-sandbox"
{
__noChroot = true;
}
Expand Down
17 changes: 8 additions & 9 deletions pkgs/tools/nix/info/sandbox.nix
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
let
pkgs = import <nixpkgs> {};
in pkgs.runCommand "diagnostics-sandbox"
{ }
''
set -x
# no cache: ${toString builtins.currentTime}
test -d "$(dirname "$out")/../var/nix"
touch $out
''
pkgs = import <nixpkgs> { };
in
pkgs.runCommand "diagnostics-sandbox" { } ''
set -x
# no cache: ${toString builtins.currentTime}
test -d "$(dirname "$out")/../var/nix"
touch $out
''