Fix breaking changes to make-derivation #51183
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
version is set in lots of places but might not need to be in a name. Alternative to NixOS#50364.
When separateDebugInfo = true & !stdenv.hostPlatform.isLinux, we always gave an error. There is no reason to do this. Instead, just don’t add separate debug info when we aren’t on Linux.
This reverts commit 1e99582.
Some trivial builders use the name attr to choose the exec name
produced. For example nixos-install,
nixos-install = makeProg {
name = "nixos-install";
src = ./nixos-install.sh;
nix = config.nix.package.out;
path = makeBinPath [ nixos-enter ];
};
When cross compiling, this puts the prog in,
/bin/nixos-install-powerpc64le-unknown-linux-gnu
GrahamcOfBorg
added
6.topic: python
6.topic: stdenv
matthewbauer
commented
Nov 28, 2018
|
|
||
| fixedOutputDrv = attrs ? outputHash; | ||
| noNonNativeDeps = builtins.length (depsBuildTarget ++ depsBuildTargetPropagated | ||
| ++ depsHostHost ++ depsHostHostPropagated | ||
| ++ buildInputs ++ propagatedBuildInputs | ||
| ++ depsTargetTarget ++ depsTargetTargetPropagated) == 0; | ||
| runtimeSensativeIfFixedOutput = fixedOutputDrv -> !noNonNativeDeps; | ||
| dontAddHostSuffix = attrs ? outputHash && !noNonNativeDeps || stdenv.cc == null; |
There was a problem hiding this comment.
@Ericson2314 Does this preserve the original condition? I'm confused as to whether you want the suffix when you have non-native deps or not?
There was a problem hiding this comment.
Yeah sorry my original booleans are a bit obtuse because I also wanted to use this to make a warning saying fixed-output derivations shouldn't have run-time deps.
In the case that they do, the suffix should appear because the run-time deps make the derivation host-sensative.
There was a problem hiding this comment.
sensative
-> "sensitive" !!
Typo is no big deal, but thought I'd say something since it's the spelling also used for the variable name 😿 .
There was a problem hiding this comment.
Notice that too! I also like dontAddHostSuffix because it's a little bit clearer on what is actually happening.
|
@GrahamcOfBorg build stdenv hello pkgsCross.aarch64-multiplatform.hello |
|
Success on aarch64-linux (full log) Attempted: stdenv, hello, pkgsCross.aarch64-multiplatform.hello Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-linux (full log) Attempted: stdenv, hello, pkgsCross.aarch64-multiplatform.hello Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: stdenv, hello, pkgsCross.aarch64-multiplatform.hello Partial log (click to expand)
|
|
cc @Synthetica9 @dtzWill (and @Ericson2314 has already been pinged) whose changes have been reverted here, if I read correctly the blame. Meta-point (I'm not familiar with the changes this PR makes, so this is not an opinion on these specific changes): I personally totally disagree with
My reasons are simple:
Anyway, all that to say that a decision of “make-derivation should be stable even between releases” vs. “make-derivation can be backwards-compatibility-broken between releases” should be done via a RFC, once the RFC-process-update RFC will be done, so that all arguments in favor and against are exposed and the decision can be considered as chosen by the community as a whole. |
|
Yes I had no problem with the specific changes here, but also disagree with that justification as @Ekleog says. |
|
Commit a56fe05 is causing (bisected by @cleverca22, after wanting to demonstrate cross comp in #nixos): |
|
Yes tracking that in #53587. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
A few breaking changes have been introduced to make-derivation recently. IMO make-derivation shouldn't ever change in what it accepts from release to release. There's too many places where this is used both internally and externally.
So this restores a few things:
substituteAllwherenameis often used for things like binary names. Try cross compiling nixos-install for example.substituteAlland theversionattr. No reason to assert here.