shellFor: provide build tools for all packages when exactDeps = false. Fix #1753. #1769

jbgi · 2022-11-04T14:34:33Z

This fix #1753.

also only depends on hsc2hs from ghcEnv to avoid collisions when using devshell (haskellLib.devshellFor).

michaelpj · 2022-11-04T15:12:57Z

builder/shell-for.nix

@@ -167,7 +167,9 @@ in
    buildInputs = systemInputs
      ++ mkDrvArgs.buildInputs or [];
    nativeBuildInputs = [ ghcEnv ]
-      ++ nativeBuildInputs
+      # We remove hsc2hs from dependencies provided by packages `build-tools`
+      # since it is already provided by `ghcEnv` (avoid collisions).


This is safe because... hsc2hs comes with GHC, so we can't end up in a situation where packages depend on multiple different versions? or what?

yes hsc2hs comes with GHC, and ghcEnv binaries are first in PATH so hsc2hs from ghcEnv has always been the one used anyway.

michaelpj · 2022-11-04T15:13:43Z

Interesting. I'm kind of surprised we don't get a collision error in shellFor itself, if it's pulling in multiple copied of hsc2hs. Do you know why that isn't happening @jbgi ?

I'd like an opinion from @hamishmack on this one also.

jbgi · 2022-11-04T15:32:59Z

I'm kind of surprised we don't get a collision error in shellFor itself, if it's pulling in multiple copied of hsc2hs. Do you know why that isn't happening @jbgi ?

yes because shellFor does not try to copy all binaries into a single output, so multiple hsc2hs are available from $PATH in this case (but the first one is from ghcEnv).

michaelpj · 2022-11-04T15:39:09Z

yes because shellFor does not try to copy all binaries into a single output, so multiple hsc2hs are available from $PATH in this case (but the first one is from ghcEnv).

Ah I see. I wonder if this is something we should also check for? Presumably such cases of PATH shadowing might lead to bugs? More generally, I guess we might behave badly if we have two packages that need to be built locally which need different versions of a build tool!

michaelpj · 2022-11-04T17:50:25Z

I just happened to spot this in cardano-node, perhaps we will be able to remove it after this PR? https://github.com/input-output-hk/cardano-node/blob/master/nix/haskell.nix#L188

jbgi · 2022-11-04T18:07:57Z

I just happened to spot this in cardano-node, perhaps we will be able to remove it after this PR?

If this was done only for the nix-shell then yes, but probably not, need to test.

jbgi · 2022-11-04T18:09:49Z

If this was done only for the nix-shell then yes, but probably not, need to test.

if not then maybe instead of my patch we need to filter out hsc2hs at the component level.

When not using `exactDeps` cabal may try to build arbitrary dependencies so in this case we need to provide the build tools for all of hsPkgs.

since it is provided by ghc env, which is first in PATH. This avoid possible collisions when using eg. devshells, which fuse all binaries into a single output directory.

jbgi · 2022-11-05T21:47:35Z

I moved the filtering-out of hsc2hs to comp-builder in last commit. However I'm not totally sure:
this old comment by @angerman points to the ability to use a patched version of hsc2hs via build-tools (ability that would be removed, if it existed, by my commit): #214 (comment)

However I doubt this ability still exists: ghc-env is always first in nativeBuildInputs, hence also first in PATH (so that the one from build-tools would never be used):

haskell.nix/builder/comp-builder.nix

Lines 388 to 390 in 7629276

    
           nativeBuildInputs = 
        
             [shellWrappers buildPackages.removeReferencesTo] 
        
             ++ executableToolDepends;

And in any case, this config flag indicates that, at least for cross-compile, only the hsc2hs from ghc is intended to be used:

haskell.nix/builder/comp-builder.nix

Line 175 in 7629276

"--with-hsc2hs=${ghc.targetPrefix}hsc2hs"

angerman · 2022-11-06T23:33:06Z

That's an interesting obersavtion. Should (explicitly specified) build-tools have precedence over the things that are shipped with GHC? That might actually be the correct thing to do.

As for the cross case. Thats mostly because we want tooling consistency and GHC's build system will prefix all produced binaries with the target prefix. I believe you could even plug in another hsc2hs just fine.

hamishmack · 2022-11-07T02:41:41Z

bors try

hamishmack · 2022-11-07T02:51:55Z

I guess in the future we could try removing hsc2hs from the GHC derivation and try to remove its special treatment from comp-builder.nix.

Perhaps shell-for.nix could somehow pick the tool with the most recent .version in the case of clashes. That might be tricky.

There is a similar problem with pkg-config versions in that different packages could require different versions, but I think we can only easily provide one.

iohk-bors · 2022-11-07T04:49:57Z

try

Timed out.

michaelpj · 2022-11-07T10:27:10Z

I don't quite understand the issue here, could someone make a ticket for this so we don't forget?

michaelpj · 2022-11-07T10:27:29Z

bors try

michaelpj · 2022-11-07T10:58:34Z

bors try

iohk-bors · 2022-11-07T10:58:38Z

try

Already running a review

iohk-bors · 2022-11-07T12:42:36Z

try

Timed out.

jbgi · 2022-11-07T14:11:58Z

bors try

iohk-bors · 2022-11-08T00:04:29Z

try

Timed out.

hamishmack · 2022-11-08T08:58:08Z

I added an issue for supporting build-tool-depends: hsc2hs:hsc2hs better #1772

#1769 added all the tool dependencies of the hsPkgs in a project to the shell. This is great for cabal projects where the set of packages will be limited to those in the plan. It allows these packages to be rebuilt by cabal. For stack projects however this set is much larger (all of stackage), likely to include unwanted tools and tools that may be broken. This change adds `shell.allDeps` and defaults it to `false` for stack projects. Fixes #1793

* Add `shell.allDeps` as a fix for #1793 #1769 added all the tool dependencies of the hsPkgs in a project to the shell. This is great for cabal projects where the set of packages will be limited to those in the plan. It allows these packages to be rebuilt by cabal. For stack projects however this set is much larger (all of stackage), likely to include unwanted tools and tools that may be broken. This change adds `shell.allDeps` and defaults it to `false` for stack projects. Fixes #1793 * Added comments

jbgi requested review from hamishmack and michaelpj November 4, 2022 14:34

jbgi mentioned this pull request Nov 4, 2022

r-42 wip IntersectMBO/plutus#4954

Closed

10 tasks

michaelpj reviewed Nov 4, 2022

View reviewed changes

jbgi force-pushed the nonExactDeps-need-all-build-tools branch from fbfd86f to c699866 Compare November 4, 2022 15:35

jbgi force-pushed the nonExactDeps-need-all-build-tools branch from c699866 to 01f822f Compare November 4, 2022 16:41

jbgi added 2 commits November 5, 2022 19:15

shellFor: provide build tools for all packages when exactDeps = false

0426fa1

When not using `exactDeps` cabal may try to build arbitrary dependencies so in this case we need to provide the build tools for all of hsPkgs.

comp-builder: exclude hsc2hs from build-tools as it is never used,

9289103

since it is provided by ghc env, which is first in PATH. This avoid possible collisions when using eg. devshells, which fuse all binaries into a single output directory.

jbgi force-pushed the nonExactDeps-need-all-build-tools branch from 01f822f to 9289103 Compare November 5, 2022 18:29

iohk-bors bot added a commit that referenced this pull request Nov 7, 2022

Try #1769:

24be276

hamishmack approved these changes Nov 7, 2022

View reviewed changes

michaelpj closed this Nov 7, 2022

iohk-bors bot added a commit that referenced this pull request Nov 7, 2022

Try #1769:

25684f3

michaelpj reopened this Nov 7, 2022

iohk-bors bot added a commit that referenced this pull request Nov 7, 2022

Try #1769:

ade64e8

hamishmack merged commit cebbb6d into master Nov 8, 2022

iohk-bors bot deleted the nonExactDeps-need-all-build-tools branch November 8, 2022 02:24

hamishmack mentioned this pull request Nov 29, 2022

Add shell.allDeps as a fix for #1793 #1794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shellFor: provide build tools for all packages when exactDeps = false. Fix #1753. #1769

shellFor: provide build tools for all packages when exactDeps = false. Fix #1753. #1769

jbgi commented Nov 4, 2022 •

edited

Loading

michaelpj Nov 4, 2022

jbgi Nov 4, 2022

michaelpj commented Nov 4, 2022

jbgi commented Nov 4, 2022 •

edited

Loading

michaelpj commented Nov 4, 2022

michaelpj commented Nov 4, 2022

jbgi commented Nov 4, 2022

jbgi commented Nov 4, 2022

jbgi commented Nov 5, 2022 •

edited

Loading

angerman commented Nov 6, 2022

hamishmack commented Nov 7, 2022

hamishmack commented Nov 7, 2022

iohk-bors bot commented Nov 7, 2022

michaelpj commented Nov 7, 2022

michaelpj commented Nov 7, 2022

michaelpj commented Nov 7, 2022

iohk-bors bot commented Nov 7, 2022

iohk-bors bot commented Nov 7, 2022

jbgi commented Nov 7, 2022

iohk-bors bot commented Nov 8, 2022

hamishmack commented Nov 8, 2022

shellFor: provide build tools for all packages when exactDeps = false. Fix #1753. #1769

shellFor: provide build tools for all packages when exactDeps = false. Fix #1753. #1769

Conversation

jbgi commented Nov 4, 2022 • edited Loading

michaelpj Nov 4, 2022

Choose a reason for hiding this comment

jbgi Nov 4, 2022

Choose a reason for hiding this comment

michaelpj commented Nov 4, 2022

jbgi commented Nov 4, 2022 • edited Loading

michaelpj commented Nov 4, 2022

michaelpj commented Nov 4, 2022

jbgi commented Nov 4, 2022

jbgi commented Nov 4, 2022

jbgi commented Nov 5, 2022 • edited Loading

angerman commented Nov 6, 2022

hamishmack commented Nov 7, 2022

hamishmack commented Nov 7, 2022

iohk-bors bot commented Nov 7, 2022

try

michaelpj commented Nov 7, 2022

michaelpj commented Nov 7, 2022

michaelpj commented Nov 7, 2022

iohk-bors bot commented Nov 7, 2022

try

iohk-bors bot commented Nov 7, 2022

try

jbgi commented Nov 7, 2022

iohk-bors bot commented Nov 8, 2022

try

hamishmack commented Nov 8, 2022

jbgi commented Nov 4, 2022 •

edited

Loading

jbgi commented Nov 4, 2022 •

edited

Loading

jbgi commented Nov 5, 2022 •

edited

Loading