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

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Jun 27, 2022

this commit doesn't change any hashes yet

this fixes overriding cargoSha256 and other attrs

so no need to overriding cargoDeps anymore

will drop the first 2 commits before merge

Closes #107070

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Artturin added 3 commits June 27, 2022 14:43
…rgoSha256'

will convert the whole file to use finalAttrs
this commit doesn't change any hashes yet

this fixes overriding cargoSha256 and other attrs

so no need to overriding cargoDeps anymore
Copy link
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

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

seems to work fine. just a few nitpicks that might not even affect normal operation though.

else if cargoLock != null then importCargoLock cargoLock
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

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

Comment on lines +82 to +83
maybeSetStr = x: lib.optionalString (previousAttrs ? ${x}) previousAttrs.${x};
maybeSetListFromPrevious = x: lib.optionals (previousAttrs ? ${x}) previousAttrs.${x};
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

Comment on lines +62 to +63
if cargoVendorDir != null then null
else if cargoLock != null then importCargoLock cargoLock
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

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?


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.


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.

@9999years
Copy link
Contributor

Anything needed for this? It would be really nice to be able to override cargoHash and similar.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: rust 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust: cannot determine cargoSha256
4 participants