buildRustPackage: use cargoHash from finalAttrs

[Prince213]

Related to #415397.

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[ShamrockLee]

This is a significant change as it shapes the <pkg>.overrideAttrs interface of the result packages. We gotta build consensus about how we would like to overrride cargoDeps before merging.

[RossSmyth]

It is not very productive to just say "there are things to consider." Of course there, but what are the alternatives? What are you preferred alternatives? Use your words rather than just saying "we need consensus." That is why I closed my last PR you commented on as I am very tired of Nixpkgs reviewers with unhelpful, unproductive comments. Please, inform us what the alternatives are.

[ShamrockLee]

@RossSmyth You are right; I should provide alternatives to make the review constructive. Thank you for sharing the reason behind your closing PR and help me improve my reviewing.

@Prince213 I apologize for blocking this PR with non-informative review.

Here are some options I could think of:

• The first decision would be whether to override cargoDeps with attributes on the main result package, such as cargoHash and cargoLock.
  • If yes, we'll need this merged, but then it would make more sense to reference cargoLock and vendorDir from finalAttrs, too.
  • If no, we'll add the above attributes into excludeDrvArgNames and treat them as syntax sugar to define the cargoDeps. All the overriding will go straight to cargoDeps. This is more simple implementation-wise, but requires more effort for users to switch between different ways of defining cargoDeps, especially because of the coupling between the main package and cargoDeps regarding source unpacking and patching.
• If we choose to override cargoHash and its siblings, the next decision would be "how to override other attributes of cargoDeps," considering the possibility for switching the way of defining cargoDeps.
  • A straightforward answer would be "just override cargoDeps", but switching the way of defining cargoDeps could break previous overrides.
  • Another way is to have something like passthru.defaultCargoDeps.fromCargoVendor, passthru.defaultCargoDeps.fromCargoLock or something, and make the cargoDeps attributes reference finalAttrs.defaultCargoDeps.<choice>. This is more extensible and could dynamically switch between ways of definition without breaking, but it is also more complex and require dedicated documentation.

[Prince213]

Ideally we'd allow overriding both cargoDeps and cargoHash, with the overriding cargoDeps take precedence over fetchCargoVendor { hash = overriding cargoHash; }.

[ShamrockLee]

Ideally we'd allow overriding both cargoDeps and cargoHash, with the overriding cargoDeps take precedence over fetchCargoVendor { hash = overriding cargoHash; }.

So we are limiting the overrideAttrs interface to cargoHash and cargoHash (and the shared unpacking and patcching attributes). Is that correct?

[MattSturgeon]

From my perspective, if the attr is passed to mkDerivation, then I'd default to reading it from finalAttrs instead of args. I'd need a specific reason not to, rather than a specific reason to.

In effect, the attr is already overridable. We're just asking whether other parts of the package should read the original value or the final value.

[Prince213]

So we are limiting the overrideAttrs interface to cargoHash and cargoHash (and the shared unpacking and patcching attributes). Is that correct?

I'm rather new to this field so please take my words with a grain of salt.

My anticipation is that we do the following:

• make fetchCargoVendor override-able, so we can do package.overrideAttrs { cargoDeps = previousAttrs.cargoDeps.override { hash = ""; } }
• let buildRustPackage construct cargoDeps from finalAttrs, so just changing cargoHash and friends would work, while when also overriding cargoDeps, the user provided will take precedence over the one we constructed.

[ShamrockLee]

• make fetchCargoVendor override-able, so we can do package.overrideAttrs { cargoDeps = previousAttrs.cargoDeps.override { hash = ""; } }

Please use cargoDeps.overrideAttrs instead. We don't want extra custom overriders.

• let buildRustPackage construct cargoDeps from finalAttrs, so just changing cargoHash and friends would work, while we also overriding cargoDeps, the user provided will take precedence over the one we constructed.

The problem is, if we reference cargoHash and friends from finalAttrs, do we want to automatically decide on the way to define cargoDeps based on finalAttrs? (Since you are changing the conditions in the if-ellse statement, you are doing so.) If so, the cargoDeps overriding is likely to break when automatic switching happens. That's why I propose to override passthru.cargoDepsFrom.<name> to avoid breakage. (This would be beneficial also when we choose not to switch automatically but still construct cargoDeps from the main package's finalAttrs.)

[Prince213]

Please use cargoDeps.overrideAttrs instead. We don't want extra custom overriders.

I think that also works. But it will be under hash instead of cargoHash. (a minor issue)

The problem is, if we reference cargoHash and friends from finalAttrs, do we want to automatically decide on the way to define cargoDeps based on finalAttrs?

I don't think I quite understand this. The definition we have here for cargoDeps is just an initial version and user overrides will take precedence right?

If so, the cargoDeps overriding is likely to break when automatic switching happens.

Please elaborate.

[ShamrockLee]

The GitHub UI refuses to let me comment on some non-changing code sections, so I'll explain it here:

Here is the initial definition of cargoDeps:

nixpkgs/pkgs/build-support/rust/build-rust-package/default.nix

Lines 104 to 130 in cdb722f

	cargoDeps =
	if cargoVendorDir != null then
	null
	else if cargoDeps != null then
	cargoDeps
	else if cargoLock != null then
	importCargoLock cargoLock
	else if args.cargoHash or null == null then
	throw "cargoHash, cargoVendorDir, cargoDeps, or cargoLock must be set"
	else
	fetchCargoVendor (
	{
	inherit
	src
	srcs
	sourceRoot
	cargoRoot
	preUnpack
	unpackPhase
	postUnpack
	;
	name = cargoDepsName;
	patches = cargoPatches;
	hash = args.cargoHash;
	}
	// depsExtraArgs
	);

The expresions after each if are the conditions, and the expressions after each then and the final else are the candidates of cargoDeps value.

It is totally fine to reference finalAttrs when defining the candidates. The tricky part is when we reference finalAttrs in the condition, as it will switch the candidates based on finalAttrs.

Since the choice (and switching) of candidates happens before the overriding (tricky, but it's how finalAttrs works), The overriding applied to cargoDeps will apply to the final selected candidate. As different candidates offer different override interfaces, the overriding could fail after switching.

More discussion needed. Still approve of the concept.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/why-isnt-the-convention-in-nixpkgs-that-hash-is-overridable-too-along-with-version-so-package-overrides-would-actually-be-useful/75634/11

[ShamrockLee]

The diff looks good.

Could we add some test cases for the cargoHash overriding via overrideAttrs or referenced from finalAttrs in test/overriding.nix?

@NixOS/rust, should overriding/evaluation tests for buildRustPackage be added to the global test/overriding.nix, or should we add files under directory pkgs/build-support/rust/test?

[winterqt]

IMO pkgs/build-support/rust/test.

[winterqt]

For my own understanding: we didn't put cargoHash in the args unpacking because then functionArgs in extendMkDerivation would be affected, right? (If my understanding is correct, can someone clarify why this behavior was desirable at the time?)

[winterqt]

(Is there a reason we don't have e.g. cargoDepsName here, which we don't use directly in the arguments to constructDrv?)

[winterqt]

e.g. we do have depsExtraArgs here, which is used in the same way as cargoDepsName