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

rustPlatform.buildRustPackage: support finalAttrs style #194475

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
149 changes: 76 additions & 73 deletions pkgs/build-support/rust/build-rust-package/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -17,66 +17,56 @@
, windows
}:

{ name ? "${args.pname}-${args.version}"

# Name for the vendored dependencies tarball
, cargoDepsName ? name

, src ? null
, srcs ? null
, preUnpack ? null
, unpackPhase ? null
, postUnpack ? null
, cargoPatches ? []
, patches ? []
, sourceRoot ? null
, logLevel ? ""
, buildInputs ? []
, nativeBuildInputs ? []
, cargoUpdateHook ? ""
, cargoDepsHook ? ""
, buildType ? "release"
, meta ? {}
, cargoLock ? null
, cargoVendorDir ? null
, checkType ? buildType
, buildNoDefaultFeatures ? false
, checkNoDefaultFeatures ? buildNoDefaultFeatures
, buildFeatures ? [ ]
, checkFeatures ? buildFeatures
, useNextest ? false
, auditable ? true

, depsExtraArgs ? {}

# Toggles whether a custom sysroot is created when the target is a .json file.
, __internal_dontAddSysroot ? false

# Needed to `pushd`/`popd` into a subdir of a tarball if this subdir
# contains a Cargo.toml, but isn't part of a workspace (which is e.g. the
# case for `rustfmt`/etc from the `rust-sources).
# Otherwise, everything from the tarball would've been built/tested.
, buildAndTestSubdir ? null
, ... } @ args:

assert cargoVendorDir == null && cargoLock == null
-> !(args ? cargoSha256 && args.cargoSha256 != null) && !(args ? cargoHash && args.cargoHash != null)
-> throw "cargoSha256, cargoHash, cargoVendorDir, or cargoLock must be set";
assert buildType == "release" || buildType == "debug";
let rustOverride =

finalAttrs: previousAttrs:

let
cargoPatches = finalAttrs.cargoPatches or [];
buildType = finalAttrs.buildType or "release";
checkType = finalAttrs.checkType or buildType;
cargoLock = finalAttrs.cargoLock or null;
cargoVendorDir = finalAttrs.cargoVendorDir or null;
buildNoDefaultFeatures = finalAttrs.buildNoDefaultFeatures or false;
checkNoDefaultFeatures = finalAttrs.checkNoDefaultFeatures or buildNoDefaultFeatures;
buildFeatures = finalAttrs.buildFeatures or [ ];
checkFeatures = finalAttrs.checkFeatures or buildFeatures;
useNextest = finalAttrs.useNextest or false;
auditable = finalAttrs.auditable or true;
depsExtraArgs = finalAttrs.depsExtraArgs or { };

# Toggles whether a custom sysroot is created when the target is a .json file.
__internal_dontAddSysroot = finalAttrs.__internal_dontAddSysroot or false;

# Needed to `pushd`/`popd` into a subdir of a tarball if this subdir
# contains a Cargo.toml, but isn't part of a workspace (which is e.g. the
# case for `rustfmt`/etc from the `rust-sources).
# Otherwise, everything from the tarball would've been built/tested.
buildAndTestSubdir = finalAttrs.buildAndTestSubdir or null;

cargoDeps =
assert cargoVendorDir == null && cargoLock == null
-> !(finalAttrs.cargoSha256 or null != null) && !(finalAttrs.cargoHash or null != null)
-> throw "One of cargoSha256, cargoHash, cargoVendorDir or cargoLock must be set";
if cargoVendorDir != null then null
else if cargoLock != null then importCargoLock cargoLock
else fetchCargoTarball ({
inherit src srcs sourceRoot preUnpack unpackPhase postUnpack cargoUpdateHook;
name = cargoDepsName;
src = finalAttrs.src or "";
srcs = finalAttrs.srcs or null;
sourceRoot = finalAttrs.sourceRoot or null;
preUnpack = finalAttrs.preUnpack or null;
unpackPhase = finalAttrs.unpackPhase or null;
# TODO using previousAttrs here as we otherwise trigger rebuilds for all
# FOD fetcher users as finalAttrs.postUnpack is prefixed below.
postUnpack = previousAttrs.postUnpack or null;
Comment on lines +59 to +61
Copy link
Member Author

Choose a reason for hiding this comment

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

Side effect of #221093. This is not ideal; I guess one solution would be to accept the rebuilds, but maybe there is a different approach here.

Copy link
Member

Choose a reason for hiding this comment

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

Attributes with null values are completely removed from the .drv, so this doesn't look like a mass rebuild to me.

Copy link
Member Author

@amesgen amesgen Apr 12, 2023

Choose a reason for hiding this comment

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

Here is a concrete example:

With this PR in its current state (48e9de7) as well as before this PR (fe2ecaf):

 $ nix-instantiate -A cargo
/nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv

After applying this diff

diff --git a/pkgs/build-support/rust/build-rust-package/default.nix b/pkgs/build-support/rust/build-rust-package/default.nix
index f8e1eef2e93..6ca8a730d59 100644
--- a/pkgs/build-support/rust/build-rust-package/default.nix
+++ b/pkgs/build-support/rust/build-rust-package/default.nix
@@ -58,7 +58,7 @@ let
       unpackPhase = finalAttrs.unpackPhase or null;
       # TODO using previousAttrs here as we otherwise trigger rebuilds for all
       # FOD fetcher users as finalAttrs.postUnpack is prefixed below.
-      postUnpack = previousAttrs.postUnpack or null;
+      postUnpack = finalAttrs.postUnpack or null;
       cargoUpdateHook = finalAttrs.cargoUpdateHook or "";
       # Name for the vendored dependencies tarball
       name = finalAttrs.cargoDepsName or finalAttrs.name or "${finalAttrs.pname}-${finalAttrs.version}";

we get

 $ nix-instantiate -A cargo
/nix/store/3msgx9jda9kc6hvfl7vgf9b8nsbm5g6y-cargo-1.68.2.drv

Diff:

 $ nix-diff /nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv /nix/store/3msgx9jda9kc6hvfl7vgf9b8nsbm5g6y-cargo-1.68.2.drv
- /nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv:{out}
+ /nix/store/3msgx9jda9kc6hvfl7vgf9b8nsbm5g6y-cargo-1.68.2.drv:{out}
• The input derivation named `cargo` differs
  - /nix/store/qcsj2rh9k0dgig555kb12vx6zrzsw2nr-cargo.drv:{out}
  + /nix/store/fmvz39xbarfgssly8j6lzjp5dcxpgwxb-cargo.drv:{out}
  • The input derivation named `cargo-auditable-0.6.1` differs
    - /nix/store/7gkiqcx8qigih6pzga18ps6w5dpv3n33-cargo-auditable-0.6.1.drv:{out}
    + /nix/store/lnxzsh3ij88gp0vzj0628p1dn4gd2irj-cargo-auditable-0.6.1.drv:{out}
    • The input derivation named `cargo-auditable-0.6.1-vendor.tar.gz` differs
      - /nix/store/n67n6kbyd0ch4v88wwra9fy57p6mr263-cargo-auditable-0.6.1-vendor.tar.gz.drv:{out}
      + /nix/store/wkvcmy569xq0lgwbfvismc7f1lwb8qhb-cargo-auditable-0.6.1-vendor.tar.gz.drv:{out}
      • The environments do not match:
          + postUnpack=eval "$cargoDepsHook"

export RUST_LOG=

    • Skipping environment comparison
  • Skipping environment comparison
• Skipping environment comparison

Does that demonstrate the problem more clearly?

cargoUpdateHook = finalAttrs.cargoUpdateHook or "";
# Name for the vendored dependencies tarball
name = finalAttrs.cargoDepsName or finalAttrs.name or "${finalAttrs.pname}-${finalAttrs.version}";
patches = cargoPatches;
} // lib.optionalAttrs (args ? cargoHash) {
hash = args.cargoHash;
} // lib.optionalAttrs (args ? cargoSha256) {
sha256 = args.cargoSha256;
} // lib.optionalAttrs (finalAttrs ? cargoHash) {
hash = finalAttrs.cargoHash;
} // lib.optionalAttrs (finalAttrs ? cargoSha256) {
sha256 = finalAttrs.cargoSha256;
} // depsExtraArgs);

target = rust.toRustTargetSpec stdenv.hostPlatform;
Expand All @@ -89,24 +79,35 @@ let
(lib.removeSuffix ".json" (builtins.baseNameOf "${target}"))
else target;

sysroot = callPackage ./sysroot { } {
sysroot = lib.throwIf (finalAttrs.doCheck or false) ''
Tests don't currently work for `no_std`, and all custom sysroots are
currently built without `std`. See https://os.phil-opp.com/testing/ for more
information.
'' (callPackage ./sysroot { } {
inherit target shortTarget;
RUSTFLAGS = args.RUSTFLAGS or "";
originalCargoToml = src + /Cargo.toml; # profile info is later extracted
};
RUSTFLAGS = previousAttrs.RUSTFLAGS or "";
originalCargoToml = finalAttrs.src + /Cargo.toml; # profile info is later extracted
});

in

# Tests don't currently work for `no_std`, and all custom sysroots are currently built without `std`.
# See https://os.phil-opp.com/testing/ for more information.
assert useSysroot -> !(args.doCheck or true);
{
removeFromBuilderEnv = [ "depsExtraArgs" "cargoUpdateHook" "cargoLock" ];

RUSTFLAGS =
if useSysroot then
lib.optionalString useSysroot "--sysroot ${sysroot} "
+ (previousAttrs.RUSTFLAGS or "")
else
previousAttrs.RUSTFLAGS or null;

stdenv.mkDerivation ((removeAttrs args [ "depsExtraArgs" "cargoUpdateHook" "cargoLock" ]) // lib.optionalAttrs useSysroot {
RUSTFLAGS = "--sysroot ${sysroot} " + (args.RUSTFLAGS or "");
} // {
inherit buildAndTestSubdir cargoDeps;
inherit cargoDeps;

cargoBuildType = buildType;
buildAndTestSubdir = previousAttrs.buildAndTestSubdir or null;

cargoBuildType =
assert buildType == "release" || buildType == "debug";
Copy link
Member

Choose a reason for hiding this comment

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

Can we move the assserts after the inputs? thats a bit cleaner than inlining them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to the evaluation model of finalAttrs, the assertions can not be kept in the place they were before. Citing from the PR description:

Asserts involving attributes from finalAttrs have to be "hidden" inside of values. Right now, I moved them to the respective attributes/variables; another option would be to put them into a dedicated asserts attribute, i.e. asserts = assert (all asserts here); "";.

buildType;

cargoCheckType = checkType;

Expand All @@ -120,7 +121,7 @@ stdenv.mkDerivation ((removeAttrs args [ "depsExtraArgs" "cargoUpdateHook" "carg

patchRegistryDeps = ./patch-registry-deps;

nativeBuildInputs = nativeBuildInputs ++ lib.optionals auditable [
nativeBuildInputs = (previousAttrs.nativeBuildInputs or [ ]) ++ lib.optionals auditable [
(cargo-auditable-cargo-wrapper.override {
inherit cargo cargo-auditable;
})
Expand All @@ -132,32 +133,34 @@ stdenv.mkDerivation ((removeAttrs args [ "depsExtraArgs" "cargoUpdateHook" "carg
rustc
];

buildInputs = buildInputs
buildInputs = (previousAttrs.buildInputs or [ ])
++ lib.optionals stdenv.hostPlatform.isDarwin [ libiconv ]
++ lib.optionals stdenv.hostPlatform.isMinGW [ windows.pthreads ];

patches = cargoPatches ++ patches;
patches = cargoPatches ++ (previousAttrs.patches or [ ]);

PKG_CONFIG_ALLOW_CROSS =
if stdenv.buildPlatform != stdenv.hostPlatform then 1 else 0;

postUnpack = ''
eval "$cargoDepsHook"

export RUST_LOG=${logLevel}
'' + (args.postUnpack or "");
export RUST_LOG=${finalAttrs.logLevel or ""}
'' + (previousAttrs.postUnpack or "");

configurePhase = args.configurePhase or ''
configurePhase = previousAttrs.configurePhase or ''
runHook preConfigure
runHook postConfigure
'';

doCheck = args.doCheck or true;
doCheck = previousAttrs.doCheck or true;

strictDeps = true;

meta = {
# default to Rust's platforms
platforms = rustc.meta.platforms;
} // meta;
})
} // (previousAttrs.meta or {});
};

in args: (stdenv.mkDerivation args).overrideAttrs rustOverride
Copy link
Member

@jtojnar jtojnar Apr 12, 2023

Choose a reason for hiding this comment

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

Rather than passing the buildRustPackage to mkDerivation and then removing them with removeFromBuilderEnv. You could remove them here and then pass them to rustOverride.

diff --git a/pkgs/build-support/rust/build-rust-package/default.nix b/pkgs/build-support/rust/build-rust-package/default.nix
index b7ae02daae0..23ae8f6b6ac 100644
--- a/pkgs/build-support/rust/build-rust-package/default.nix
+++ b/pkgs/build-support/rust/build-rust-package/default.nix
@@ -19,13 +19,14 @@
 
 let rustOverride =
 
+rustAttrs:
 finalAttrs:
 
 let
   cargoPatches = finalAttrs.cargoPatches or [];
   buildType = finalAttrs.buildType or "release";
   checkType = finalAttrs.checkType or buildType;
-  cargoLock = finalAttrs.cargoLock or null;
+  cargoLock = rustAttrs.cargoLock or null;
   cargoVendorDir = finalAttrs.cargoVendorDir or null;
   buildNoDefaultFeatures = finalAttrs.buildNoDefaultFeatures or false;
   checkNoDefaultFeatures = finalAttrs.checkNoDefaultFeatures or buildNoDefaultFeatures;
@@ -33,7 +34,7 @@ let
   checkFeatures = finalAttrs.checkFeatures or buildFeatures;
   useNextest = finalAttrs.useNextest or false;
   auditable = finalAttrs.auditable or false; # TODO: change to true
-  depsExtraArgs = finalAttrs.depsExtraArgs or { };
+  depsExtraArgs = rustAttrs.depsExtraArgs or { };
 
   # Toggles whether a custom sysroot is created when the target is a .json file.
   __internal_dontAddSysroot = finalAttrs.__internal_dontAddSysroot or false;
@@ -57,7 +58,7 @@ let
       preUnpack = finalAttrs.preUnpack or null;
       unpackPhase = finalAttrs.unpackPhase or null;
       postUnpack = finalAttrs.postUnpack or null;
-      cargoUpdateHook = finalAttrs.cargoUpdateHook or "";
+      cargoUpdateHook = rustAttrs.cargoUpdateHook or "";
       # Name for the vendored dependencies tarball
       name = finalAttrs.cargoDepsName or finalAttrs.name or "${finalAttrs.pname}-${finalAttrs.version}";
       patches = cargoPatches;
@@ -85,7 +86,6 @@ let
 
 in
 
-
 previousAttrs:
 
 lib.optionalAttrs useSysroot {
@@ -95,7 +95,6 @@ lib.optionalAttrs useSysroot {
     information.
   '' ("--sysroot ${sysroot} " + (finalAttrs.RUSTFLAGS or ""));
 } // {
-  removeFromBuilderEnv = [ "depsExtraArgs" "cargoUpdateHook" "cargoLock" ];
 
   inherit cargoDeps;
 
@@ -159,4 +158,9 @@ lib.optionalAttrs useSysroot {
   } // (previousAttrs.meta or {});
 };
 
-in args: (stdenv.mkDerivation args).overrideAttrs rustOverride
+in
+args:
+let
+  derivationArgs = finalAttrs: builtins.removeAttrs (if builtins.isFunction finalAttrs then args finalAttrs else args) [ "depsExtraArgs" "cargoUpdateHook" "cargoLock" ];
+in
+(stdenv.mkDerivation derivationArgs).overrideAttrs (rustOverride args)

Edit: Getting infinite recursion 😞 Would be nice to have NixOS/nix#4154

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, accidentally running into infinite recursions also happened to me a few times 😄

6 changes: 4 additions & 2 deletions pkgs/stdenv/generic/make-derivation.nix
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,10 @@ else let
"nativeCheckInputs" "nativeInstallCheckInputs"
"__darwinAllowLocalNetworking"
"__impureHostDeps" "__propagatedImpureHostDeps"
"sandboxProfile" "propagatedSandboxProfile"]
++ lib.optional (__structuredAttrs || envIsExportable) "env"))
"sandboxProfile" "propagatedSandboxProfile"
"removeFromBuilderEnv"]
Copy link
Member

Choose a reason for hiding this comment

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

With __structuredAtts the arguments passed to mkDerivation will not end up in the environment so this will become redundant.

Copy link
Member

Choose a reason for hiding this comment

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

They would still become part of the JSON and part of the hash, causing rebuilds.
For instance, changes to cargoUpdateHook were previously masked by the FOD hash, but this reintroduces that value as an unnecessary input dependency that will cause a rebuild even if the FOD hash didn't have to change.

Similarly, you may want to have a "local" variable that only affects the passthru attributes, but isn't a passthru itself; perhaps some configuration for an automatically generated updateScript. That could only be expressed by filtering away at least one more attribute.

That said, perhaps the name could be improved by removing the Env part; what about removeFromBuilder?

It's not a pretty solution, but it's the best we can do when we don't have an alternative place for such attributes. We could have a single attribute like locals specifically for this purpose, but that doesn't help with attributes that already exist, and I don't like that it exposes an implementation detail to users. "Local" should be the default behavior of attributes at the root of the argument, but at that point it's not mkDerivation anymore.

++ lib.optional (__structuredAttrs || envIsExportable) "env"
++ (attrs.removeFromBuilderEnv or [])))
// (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
name =
let
Expand Down
13 changes: 6 additions & 7 deletions pkgs/tools/text/difftastic/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
, rustPlatform
, fetchFromGitHub
, testers
, difftastic
}:

let
Expand All @@ -14,14 +13,14 @@ let
};
in

rustPlatform.buildRustPackage rec {
rustPlatform.buildRustPackage (finalAttrs: {
pname = "difftastic";
version = "0.46.0";

src = fetchFromGitHub {
owner = "wilfred";
repo = pname;
rev = version;
repo = "difftastic";
rev = finalAttrs.version;
sha256 = "sha256-uXSmEJUpcw/PQ5I9nR1b6N1fcOdCSCM4KF0XnGNJkME=";
};

Expand All @@ -37,14 +36,14 @@ rustPlatform.buildRustPackage rec {
-p1 < ${mimallocPatch}
'';

passthru.tests.version = testers.testVersion { package = difftastic; };
passthru.tests.version = testers.testVersion { package = finalAttrs.finalPackage; };

meta = with lib; {
description = "A syntax-aware diff";
homepage = "https://github.com/Wilfred/difftastic";
changelog = "https://github.com/Wilfred/difftastic/blob/${version}/CHANGELOG.md";
changelog = "https://github.com/Wilfred/difftastic/blob/${finalAttrs.version}/CHANGELOG.md";
license = licenses.mit;
maintainers = with maintainers; [ ethancedwards8 figsoda ];
mainProgram = "difft";
};
}
})