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

buildRustPackage: fix overrideAttrs #179392

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

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

# Name for the vendored dependencies tarball
, cargoDepsName ? name

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

# Toggles whether a custom sysroot is created when the target is a .json file.

# 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.
# 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:
, ...
} @ args:

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

let

cargoDeps =
if cargoVendorDir != null then null
else if cargoLock != null then importCargoLock cargoLock
else fetchCargoTarball ({
inherit src srcs sourceRoot unpackPhase cargoUpdateHook;
name = cargoDepsName;
patches = cargoPatches;
} // lib.optionalAttrs (args ? cargoHash) {
hash = args.cargoHash;
} // lib.optionalAttrs (args ? cargoSha256) {
sha256 = args.cargoSha256;
} // depsExtraArgs);

# If we have a cargoSha256 fixed-output derivation, validate it at build time
# against the src fixed-output derivation to check consistency.
validateCargoDeps = args ? cargoHash || args ? cargoSha256;

target = rust.toRustTargetSpec stdenv.hostPlatform;
targetIsJSON = lib.hasSuffix ".json" target;
useSysroot = targetIsJSON && !__internal_dontAddSysroot;

# see https://github.com/rust-lang/cargo/blob/964a16a28e234a3d397b2a7031d4ab4a428b1391/src/cargo/core/compiler/compile_kind.rs#L151-L168
# the "${}" is needed to transform the path into a /nix/store path before baseNameOf
shortTarget = if targetIsJSON then
(lib.removeSuffix ".json" (builtins.baseNameOf "${target}"))
else target;

sysroot = callPackage ./sysroot { } {
inherit target shortTarget;
RUSTFLAGS = args.RUSTFLAGS or "";
originalCargoToml = 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);

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

cargoBuildType = buildType;

cargoCheckType = checkType;

cargoBuildNoDefaultFeatures = buildNoDefaultFeatures;

cargoCheckNoDefaultFeatures = checkNoDefaultFeatures;

cargoBuildFeatures = buildFeatures;

cargoCheckFeatures = checkFeatures;

patchRegistryDeps = ./patch-registry-deps;

nativeBuildInputs = nativeBuildInputs ++ [
cacert
git
cargoBuildHook
cargoCheckHook
cargoInstallHook
cargoSetupHook
rustc
];

buildInputs = buildInputs
# depsExtraArgs, cargoLock have to be removed otherwise 'cannot coerce a set to a string'
# with structuredAttrs this won't be needed
# also cargoUpdateHook to avoid changing hashes
(stdenv.mkDerivation (removeAttrs args [ "depsExtraArgs" "cargoLock" "cargoUpdateHook" ])).overrideAttrs (finalAttrs: previousAttrs:
let
# If we have a cargoSha256 fixed-output derivation, validate it at build time
# against the src fixed-output derivation to check consistency.
# NOTE: this seems unused
#validateCargoDeps = finalAttrs ? cargoHash || finalAttrs ? cargoSha256;

target = rust.toRustTargetSpec stdenv.hostPlatform;
targetIsJSON = lib.hasSuffix ".json" target;
useSysroot = targetIsJSON && !__internal_dontAddSysroot;

# see https://github.com/rust-lang/cargo/blob/964a16a28e234a3d397b2a7031d4ab4a428b1391/src/cargo/core/compiler/compile_kind.rs#L151-L168
# the "${}" is needed to transform the path into a /nix/store path before baseNameOf
shortTarget =
if targetIsJSON then
(lib.removeSuffix ".json" (builtins.baseNameOf "${target}"))
else target;
sysroot = callPackage ./sysroot { } {
inherit target shortTarget;
RUSTFLAGS = finalAttrs.RUSTFLAGS or "";
originalCargoToml = finalAttrs.src + /Cargo.toml; # profile info is later extracted
};

cargoDeps =
if cargoVendorDir != null then null
else if cargoLock != null then importCargoLock cargoLock
Comment on lines +62 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe these should be overridable from finalAttrs as well

else
fetchCargoTarball ({
src = finalAttrs.src or "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
src = finalAttrs.src or "";
src = finalAttrs.src or null;

probably not meaningful anyway, but keeping the default unchanged seems nicer

srcs = finalAttrs.srcs or null;
sourceRoot = finalAttrs.sourceRoot or null;
unpackPhase = finalAttrs.unpackPhase or null;
cargoUpdateHook = finalAttrs.cargoUpdateHook or args.cargoUpdateHook or "";
# Name for the vendored dependencies tarball
name = finalAttrs.cargoDepsName or finalAttrs.name or "${finalAttrs.pname}-${finalAttrs.version}";
# see comment below
#patches = finalAttrs.cargoPatches;
patches = finalAttrs.cargoPatches or [ ];
} // lib.optionalAttrs (finalAttrs ? cargoHash) {
hash = finalAttrs.cargoHash;
} // lib.optionalAttrs (finalAttrs ? cargoSha256) {
sha256 = finalAttrs.cargoSha256;
} // (args.depsExtraArgs or { }));
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be?

Suggested change
} // (args.depsExtraArgs or { }));
} // (finalAttrs.depsExtraArgs or args.depsExtraArgs or { }));


maybeSetStr = x: lib.optionalString (previousAttrs ? ${x}) previousAttrs.${x};
maybeSetListFromPrevious = x: lib.optionals (previousAttrs ? ${x}) previousAttrs.${x};
Comment on lines +82 to +83
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific reason to use these instead of

Suggested change
maybeSetStr = x: lib.optionalString (previousAttrs ? ${x}) previousAttrs.${x};
maybeSetListFromPrevious = x: lib.optionals (previousAttrs ? ${x}) previousAttrs.${x};
maybeSetStr = x: previousAttrs.${x} or "";
maybeSetListFromPrevious = x: previousAttrs.${x} or [];

or inlining them outright? looks like it'd be equivalent and eval slightly faster


# cargoBuildFeatures example:
# previous: use directly specified cargoBuildFeatures from the original derivation (the one in nixpkgs)
# final: use buildType from either the original derivation or one overriden with overrideAttrs
# default: use the default passed if none of the above are set
pickPreviousOrFinalOrDefault = previous: final: default: (previousAttrs.${previous} or finalAttrs.${final} or default);

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 -> !(finalAttrs.doCheck or true);
{
inherit buildAndTestSubdir cargoDeps;

# buildType is checked for 4 of the cargo built in types
# directly specifying cargoBuildType can be used as a escape-hatch
cargoBuildType = assert finalAttrs ? buildType -> lib.assertOneOf "buildType" finalAttrs.buildType [ "release" "debug" "test" "bench" ];
pickPreviousOrFinalOrDefault "cargoBuildType" "buildType" "release";

cargoCheckType = pickPreviousOrFinalOrDefault "cargoCheckType" "checkType" finalAttrs.cargoBuildType;

cargoBuildNoDefaultFeatures = pickPreviousOrFinalOrDefault "cargoBuildNoDefaultFeatures" "buildNoDefaultFeatures" false;

cargoCheckNoDefaultFeatures = pickPreviousOrFinalOrDefault "cargoCheckNoDefaultFeatures" "checkNoDefaultFeatures"
finalAttrs.cargoBuildNoDefaultFeatures;

cargoBuildFeatures = pickPreviousOrFinalOrDefault "cargoBuildFeatures" "buildFeatures" "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cargoBuildFeatures = pickPreviousOrFinalOrDefault "cargoBuildFeatures" "buildFeatures" "";
cargoBuildFeatures = pickPreviousOrFinalOrDefault "cargoBuildFeatures" "buildFeatures" [];

old default was [], probably better to keep it like that?


cargoCheckFeatures = pickPreviousOrFinalOrDefault "cargoCheckFeatures" "checkFeatures" finalAttrs.cargoBuildFeatures;

patchRegistryDeps = ./patch-registry-deps;

nativeBuildInputs = maybeSetListFromPrevious "nativeBuildInputs" ++ [
cacert
git
cargoBuildHook
cargoCheckHook
cargoInstallHook
cargoSetupHook
rustc
];

buildInputs = maybeSetListFromPrevious "buildInputs"
++ lib.optionals stdenv.hostPlatform.isDarwin [ libiconv ]
++ lib.optionals stdenv.hostPlatform.isMinGW [ windows.pthreads ];

patches = cargoPatches ++ patches;
# commented for now to keep drv hash from changing
#cargoPatches = maybeSetListFromPrevious "cargoPatches";
#patches = finalAttrs.cargoPatches ++ maybeSetListFromPrevious "patches";

patches = (finalAttrs.cargoPatches or [ ]) ++ maybeSetListFromPrevious "patches";

PKG_CONFIG_ALLOW_CROSS = previousAttrs.PKG_CONFIG_ALLOW_CROSS or
Copy link
Contributor

Choose a reason for hiding this comment

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

does this (technically) change the behavior of this binding? looks like previously the value from args would always be overwritten.

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

PKG_CONFIG_ALLOW_CROSS =
if stdenv.buildPlatform != stdenv.hostPlatform then 1 else 0;
postUnpack = ''
eval "$cargoDepsHook"

postUnpack = ''
eval "$cargoDepsHook"
export RUST_LOG=${finalAttrs.logLevel or ""}
'' + maybeSetStr "postUnpack";

export RUST_LOG=${logLevel}
'' + (args.postUnpack or "");
configurePhase = previousAttrs.configurePhase or ''
runHook preConfigure
runHook postConfigure
'';

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

doCheck = args.doCheck or true;
strictDeps = previousAttrs.strictDeps or true;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, previously we'd always set this to true. probably better to leave it like that anyway.


strictDeps = true;
passthru = { inherit cargoDeps; } // (previousAttrs.passthru or { });

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

meta = {
# default to Rust's platforms
platforms = rustc.meta.platforms;
} // meta;
})
} // lib.optionalAttrs useSysroot
{ RUSTFLAGS = "--sysroot ${sysroot} " + (previousAttrs.RUSTFLAGS or ""); })
18 changes: 18 additions & 0 deletions pkgs/top-level/all-packages.nix
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,24 @@ with pkgs;

deadnix = callPackage ../development/tools/deadnix { };

deadnix1 = (deadnix.overrideAttrs (finalAttrs: previousAttrs: {
version = finalAttrs.src.rev;
src = fetchFromGitHub {
owner = "astro";
repo = "deadnix";
rev = "83c42cc64d190ecb72f5929eab0f64fe88e25dc4";
sha256 = "sha256-npyHYIJW7HyGIFpCZZK+t5JM/v2LsyFhAGJxX1DXO7E=";
};

cargoSha256 = "sha256-kZ4Bd0ba63QBHLAy7WwaR6KCZ3RwuPgp3h8+PKF9LQQ=";
# no longer necessary to override cargoDeps
#cargoDeps = previousAttrs.cargoDeps.overrideAttrs (lib.const {
# name = "deadnix-vendor.tar.gz";
# inherit (finalAttrs) src;
# outputHash = "sha256-9U8cZ8+oa9n1kP3WgYSpk9deC3zBECH/YSTNA9e4jsc=";
#});
}));

dsq = callPackage ../tools/misc/dsq { buildGoModule = buildGo118Module; };

each = callPackage ../tools/text/each { };
Expand Down