make-derivation: Don't add host-suffix to fixed-output derivations names #33589

dtzWill · 2018-01-08T00:55:30Z

Not only does the suffix unnecessarily reduce sharing, but it also breaks
unpacker setup hooks (e.g. that of unzip) which identify interesting tarballs
using the file extension.

Motivation for this change

Fixes #32986.

(another from #30882!)

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Ericson2314 · 2018-01-08T07:07:11Z

pkgs/stdenv/generic/make-derivation.nix

+              # host suffix. See #32986.
+              isHostSensitive =
+                stdenv.hostPlatform != stdenv.buildPlatform &&
+                (builtins.length buildInputs != 0 ||


Ah so this part isn't actually true. Say we are building compiler-rt; it run-time depends on nothing, but cares about the host platform as much as any other platform.

In that thread, I instead meant that separately we should do a check like:

let fixedOutputDrv = args ? outputHash; noNonNativeDeps = builtins.length (depsBuildTarget ++ depsBuildTargetPropagated ++ depsHostHost ++ depsHostHostPropagated ++ buildInputs ++ propagatedBuildInputs ++ depsTargetTarget ++ depsTargetTargetPropagated) == 0; in assert fixedOutputDrv -> noNonNativeDeps;

Technically, even that isn't always necessarily true. Yes, any of those deps mean we now need host- or target-platform-specific hashes, but for pre-built stuff (bootstrapping, proprietary, etc) we do just that. So I suppose pre-built stuff could have such deps. But unless all transitive deps were also fixed-output, it would be a total pain to keep the hash correct.

In practice, for the platform-specific deps we do use, run-time deps are hooked up in a downstream deriving, e.g. with substituteAll or makeWrapper, so I don't there's a practical reason not to have that assertion. But it would be fine to leave it for a separate PR if you like, as it might require fixing miscategorized deps / is a separate issue from this one.

Ericson2314 · 2018-01-08T07:14:32Z

Oh also with this change, we can remove hacks in https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/splice.nix#L34-L40 and knock off that todo.

Sorry the conversation wasn't the clearest thing to follow, but thanks for tackling this!

Ericson2314 · 2018-01-08T07:26:30Z

Lastly, not sure weather its worth it until we go back to staging mass rebuilds, but I've been trying to linearizing the history of my PRs under https://github.com/obsidiansystems/nixpkgs/tree/ericson2314-cross-master, for easy diffing and bisecting if anything goes wrong. I just rebased this on top of that.

I fixed my own issues

Ericson2314 · 2018-01-10T00:19:29Z

phobosUnittests is a very odd thing I'm not sure what to do about.

dtzWill · 2018-01-10T00:26:28Z

@Ericson2314 sounds like a test that should be disabled on cross! Right?

Ericson2314 · 2018-01-10T00:46:41Z

@dtzWill well I disabled fixed output derivations with run-time dependencies across the board. But those test drvs' deps shouldn't be nativeBuildInputs because they're run-time dependencies....of the thing which isn't actually being built because we just run the tests and through everything away.
They're fixed output derivations which are more or less just being used for their side affects.

Ericson2314 · 2018-01-10T01:17:28Z

I suppose while we decide whether or not to reflex the restriction, we can still clean up the deps that unequivocally need it #33681 .

vcunat · 2018-01-10T08:06:16Z

I disabled fixed output derivations with run-time dependencies across the board.

This confuses me. I don't think nix allows fixed-output derivations to retain any rutime references. EDIT: ah, eval-time check for buildInputs, right...

Ericson2314 · 2018-01-11T17:25:08Z

@vcunat what do you think now? Switched back to keeping hash with run-time deps, and just making a warning.

vcunat · 2018-01-14T21:07:49Z

Changes in laziness of derivation attributes #33057 conflicted this, and it makes me wonder why you chose to do this warning so strictly. I think it's just another way of broken-ness and would better be evaluated in the same "cases" (i.e. with the same "laziness"). Maybe it would even be worth to move it to ./check-meta.nix, even though it's not about meta – but we haven't checked anything else so far, so perhaps just the name doesn't fit.

dtzWill · 2018-01-18T17:44:50Z

(I'm more or less disowning this PR, looks like @Ericson2314 is leading the way, thank you! 👍)

Not only does the suffix unnecessarily reduce sharing, but it also breaks unpacker setup hooks (e.g. that of `unzip`) which identify interesting tarballs using the file extension. This also means we can get rid of the splicing hacks for fetchers.

Ericson2314 · 2018-07-09T15:13:08Z

pkgs/stdenv/generic/make-derivation.nix

@@ -77,8 +77,13 @@ rec {

    , ... } @ attrs:

-    # TODO(@Ericson2314): Make this more modular, and not O(n^2).


I believe this about the hardening flags, but it's quite unclear so I just removed it.

Ericson2314 · 2018-07-09T16:09:14Z

I just skipped the warning for now, avoiding what @vcunat was concerned about entirely.

edolstra · 2018-07-09T17:34:40Z

What is the impact of this change on evaluation performance?

(Asking because there has been a pretty awful slowdown in Nixpkgs evaluation speed in the last year, and I suspect it's due to all the stdenv changes.)

Ericson2314 · 2018-07-09T17:56:08Z

@edolstra I did think about that here. There should be no effect on native, since the && will short circuit. [On cross it's unclear, with the length check per derivation, but the splicing nasty fetcher filtering removed.]

Ericson2314 · 2018-07-09T17:57:10Z

pkgs/stdenv/generic/make-derivation.nix

+            # suffix. But we have some weird ones with run-time deps that are
+            # just used for their side-affects. Those might as well since the
+            # hash can't be the same. See #32986.
+            (stdenv.hostPlatform != stdenv.buildPlatform && runtimeSensativeIfFixedOutput)


@edolstra this is the && I'm referring to.

dtzWill requested a review from edolstra as a code owner January 8, 2018 00:55

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 8, 2018

Ericson2314 previously requested changes Jan 8, 2018

View reviewed changes

Ericson2314 force-pushed the fix/fixed-output-cross-name branch 8 times, most recently from 0072eb5 to 6fd63d0 Compare January 9, 2018 23:22

Ericson2314 added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Jan 9, 2018

Ericson2314 force-pushed the fix/fixed-output-cross-name branch from cab7cd3 to a16c912 Compare January 10, 2018 00:51

Ericson2314 force-pushed the fix/fixed-output-cross-name branch from a16c912 to 1d94ed0 Compare January 10, 2018 03:02

Ericson2314 force-pushed the fix/fixed-output-cross-name branch 2 times, most recently from f28f4e0 to 53a4027 Compare January 11, 2018 16:42

Ericson2314 added this to After big PR in Cross compilation Jan 11, 2018

Ericson2314 force-pushed the fix/fixed-output-cross-name branch from 53a4027 to d11bc26 Compare January 11, 2018 17:12

Ericson2314 mentioned this pull request Apr 5, 2018

Target suffix added to source file names when cross-compiling breaks unzip setupHook #32986

Closed

Ericson2314 force-pushed the fix/fixed-output-cross-name branch from d11bc26 to 7f8c199 Compare July 9, 2018 14:43

GrahamcOfBorg added the 6.topic: stdenv Standard environment label Jul 9, 2018

Ericson2314 force-pushed the fix/fixed-output-cross-name branch from 7f8c199 to 218d4dc Compare July 9, 2018 15:07

Ericson2314 reviewed Jul 9, 2018

View reviewed changes

Ericson2314 merged commit cf72af3 into NixOS:master Jul 9, 2018

Ericson2314 reviewed Jul 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make-derivation: Don't add host-suffix to fixed-output derivations names #33589

make-derivation: Don't add host-suffix to fixed-output derivations names #33589

dtzWill commented Jan 8, 2018

Ericson2314 Jan 8, 2018

Ericson2314 Jan 8, 2018

Ericson2314 commented Jan 8, 2018

Ericson2314 commented Jan 8, 2018 •

edited

Ericson2314 commented Jan 10, 2018

dtzWill commented Jan 10, 2018

Ericson2314 commented Jan 10, 2018 •

edited

Ericson2314 commented Jan 10, 2018

vcunat commented Jan 10, 2018 •

edited

Ericson2314 commented Jan 11, 2018

vcunat commented Jan 14, 2018

dtzWill commented Jan 18, 2018

Ericson2314 Jul 9, 2018

Ericson2314 commented Jul 9, 2018

edolstra commented Jul 9, 2018

Ericson2314 commented Jul 9, 2018 •

edited

Ericson2314 Jul 9, 2018

		@@ -77,8 +77,13 @@ rec {

		, ... } @ attrs:

		# TODO(@Ericson2314): Make this more modular, and not O(n^2).

make-derivation: Don't add host-suffix to fixed-output derivations names #33589

make-derivation: Don't add host-suffix to fixed-output derivations names #33589

Conversation

dtzWill commented Jan 8, 2018

Motivation for this change

Things done

Ericson2314 Jan 8, 2018

Choose a reason for hiding this comment

Ericson2314 Jan 8, 2018

Choose a reason for hiding this comment

Ericson2314 commented Jan 8, 2018

Ericson2314 commented Jan 8, 2018 • edited

Ericson2314 commented Jan 10, 2018

dtzWill commented Jan 10, 2018

Ericson2314 commented Jan 10, 2018 • edited

Ericson2314 commented Jan 10, 2018

vcunat commented Jan 10, 2018 • edited

Ericson2314 commented Jan 11, 2018

vcunat commented Jan 14, 2018

dtzWill commented Jan 18, 2018

Ericson2314 Jul 9, 2018

Choose a reason for hiding this comment

Ericson2314 commented Jul 9, 2018

edolstra commented Jul 9, 2018

Ericson2314 commented Jul 9, 2018 • edited

Ericson2314 Jul 9, 2018

Choose a reason for hiding this comment

Ericson2314 commented Jan 8, 2018 •

edited

Ericson2314 commented Jan 10, 2018 •

edited

vcunat commented Jan 10, 2018 •

edited

Ericson2314 commented Jul 9, 2018 •

edited