Skip to content

Commit

Permalink
Revert "make-derivation: add disallowedReferences in strictDeps"
Browse files Browse the repository at this point in the history 
This reverts commit 8dbfb61.
Also reverts commit fc99c33.

Fixes #50915
  • Loading branch information
@matthewbauer
matthewbauer committed Nov 25, 2018
1 parent b49ed49 commit 7eeb02d
Showing 1 changed file with 0 additions and 16 deletions.
16 changes: 0 additions & 16 deletions pkgs/stdenv/generic/make-derivation.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,22 +228,6 @@ rec {
inherit doCheck doInstallCheck;

inherit outputs;
} // lib.optionalAttrs strictDeps {
# Make sure "build" dependencies don’t leak into outputs. We
# want to disallow references to depsBuildBuild,
# nativeBuildInputs, and depsBuildTarget. But depsHostHost,
# buildInputs, and depsTargetTarget is okay, so we subtract
# those from disallowedReferences in case a dependency is
# listed in multiple dependency lists. We also include
# propagated dependencies here as well.
disallowedReferences = (attrs.disallowedReferences or [])
++ (lib.subtractLists
(lib.concatLists ((lib.elemAt propagatedDependencies 0) ++
(lib.elemAt propagatedDependencies 1) ++
(lib.elemAt dependencies 1) ++
(lib.elemAt propagatedDependencies 2) ++
(lib.elemAt dependencies 2) ) )
(lib.concatLists ((lib.elemAt dependencies 0)) ) );
} // lib.optionalAttrs (stdenv.hostPlatform != stdenv.buildPlatform) {
cmakeFlags =
(/**/ if lib.isString cmakeFlags then [cmakeFlags]
Expand Down

6 comments on commit 7eeb02d

@matthewbauer
Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to attach reasoning. Here it is from @lopsided98,

This appears to be breaking kernel cross compilation (see #50915 (comment)), because the dev output refers to build perl, gcc and openssl. We could try to substitute these with host deps, but that really doesn't make sense.

If something depends on the cross compiled kernel's dev output, it is going to be cross compiled as well, and it is likely going to run the scripts in the dev output at build time. Therefore these scripts need to refer to build packages or they will probably fail.

This is not just true for the kernel; any cross compiled dev output is probably only going to be used on the build system, and therefore it doesn't make sense to disallow build references. Normally, this isn't a problem because the dev output is just headers that don't refer to anything. When there are scripts or makefiles that refer to other dependencies, this PR incorrectly breaks the build or we have to use hacks like putting /usr/bin/env in shebangs, like @Ericson2314 mentioned.

It seems to me that disallowing native build inputs in dev outputs is conceptually incorrect.

Agreed. Dev outputs are usually juts headers but there are enough corner cases to cause issues. I think the information from this is extremely valuable still - we just need to find a way to avoid the breakages. Anyway the cross-patch-shebangs branch has been reverted again! So we need to get that straightened out before we can do something like this either way. We will probably need to wait until something like NixOS/nix@3cd15c5 is widely available.

Sorry for any inconveniences! Reverted in 7eeb02d.

@delroth
Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewbauer looks like this commit broke pretty much everything: https://hydra.nixos.org/eval/1491743#tabs-now-fail

Could you revert for now so master isn't completely unusable?

@delroth
Copy link
Contributor

Choose a reason for hiding this comment

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

(Note: I don't know for sure that it's this commit which broke everything in that Hydra eval, but I've bisected at least a gnutls build failure to this commit... and I'm guessing a lot of things depend on gnutls. The batch of commits in this eval is also mostly harmless other than this one.)

@lopsided98
Copy link
Contributor

Choose a reason for hiding this comment

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

Hydra is claiming the failures are due to a broken valgrind build from weeks ago, which doesn't make any sense.

I don't know why this would break anything but, like you said, this seems most likely out of any in that evaluation.

What is the failure you are seeing with gnutls?

@delroth
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing its testsuite fail on !NixOS Linux because of not finding "socat".

Note that it also seems like this is causing massive amounts of rebuilds, so it should at least have gone into staging first...

@lopsided98
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't reproduce the gnutls failure. Building gnutls on ab88ed6 gives me /nix/store/7cwsp9yh11zr7wqri77my903mjfsyw87-gnutls-3.6.2-bin, which is already cached.

Please sign in to comment.