treewide: Modular setup hooks #40139

Ericson2314 · 2018-05-07T18:50:59Z

Motivation for this change

It's error prone to copy paste this "role" stuff everywhere. Also, the more similar the *-wrappers are, the easier it will be add a pkgconfig-wrapper as @bgamari once mentioned needing. Also some of the env hooks should just be setup hooks (setup hooks directly rather than setup hooks that define env hooks).

I don't really like this setupHooks bloat, but after #31414 we can just make these helper setup hooks dependencies, which will work fine. I'd have loved to land that PR first, but fixing it requires #26004 which would be easier (but still hard) with this, so I do this stop-gap first.

Fixes #40046

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.

matthewbauer · 2018-05-07T18:57:00Z

pkgs/development/libraries/libiconv/setup-hook.sh

-    export NIX_${role}LDFLAGS+=" -liconv"
-}
-
-addEnvHooks "$hostOffset" iconvLdflags


So were addEnvHooks ever needed here? I was not really sure what they were doing but it was used in many places.

Nope! and env hook does something per dependency. It's a way to accumulate a bunch of things which are for-eached at the end. Basically, If your hook doesn't use $1, it shouldn't be an env hook.

matthewbauer · 2018-05-07T18:59:32Z

pkgs/build-support/bintools-wrapper/default.nix

@@ -300,7 +305,7 @@ stdenv.mkDerivation {

  inherit dynamicLinker expand-response-params;

-  # for substitution in utils.sh
+  # for substitution in utils.bash


What is making these bash scripts? I know we have in the past assumed bash - but it seemed nice to make these in theory work on non-Bash shells.

Virtually all our stuff is bash-only. Given we're not on MS-DOS with max-3 file extensions, I figure it's best to be honest on use .bash not .sh :D. More seriously, editors highlight better with .bash: emacs will at least check the shebang, but many of these hook files don't have one.

Also fix some setup hooks that unnecessarily used environment hooks, which revolted in the same variable being modified too many times.

matthewbauer · 2018-05-07T19:14:32Z

Fixes #40134

I think you have the wrong number there.

Ericson2314 · 2018-05-07T19:18:17Z

fixed: #40046

matthewbauer · 2018-05-07T19:22:59Z

pkgs/build-support/bintools-wrapper/default.nix

+
+  setupHooks = [
+    ../setup-hooks/role.bash
+    ./setup-hook.sh


Could we just source this in the setup-hook.sh? That would mean we shouldn't need to add the setupHooks logic (yet).

@matthewbauer well it still needs to be part of the derivation closure which means extra hoops anyways.

matthewbauer · 2018-05-07T19:43:40Z

Awesome work!

GrahamcOfBorg · 2018-05-07T20:08:53Z

Success on x86_64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

shrinking /nix/store/f07sgxxhq5ysc92gy0yd48ibv276fick-findutils-4.6.0/libexec/code
shrinking /nix/store/f07sgxxhq5ysc92gy0yd48ibv276fick-findutils-4.6.0/libexec/bigram
gzipping man pages under /nix/store/f07sgxxhq5ysc92gy0yd48ibv276fick-findutils-4.6.0/share/man/
strip is /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/strip
stripping (with command strip and flags -S) in /nix/store/f07sgxxhq5ysc92gy0yd48ibv276fick-findutils-4.6.0/libexec  /nix/store/f07sgxxhq5ysc92gy0yd48ibv276fick-findutils-4.6.0/bin
checking for references to /build in /nix/store/f07sgxxhq5ysc92gy0yd48ibv276fick-findutils-4.6.0...
shrinking RPATHs of ELF executables and libraries in /nix/store/mmrpfl978dnl5msc5jrx25xp0yxs98n1-findutils-4.6.0-info
strip is /nix/store/d1prcspbh2qsviipvnaxizcj8l3g7fpw-bootstrap-tools/bin/strip
checking for references to /build in /nix/store/mmrpfl978dnl5msc5jrx25xp0yxs98n1-findutils-4.6.0-info...
building '/nix/store/sy38sm75r6zgai3lsmhsf355wzh52d9p-stdenv-linux.drv'...

matthewbauer · 2018-05-07T20:12:21Z

Also might want to look to see if this fixes #40013. We also have a hack for inkscape having long args in a276d51...047c937 that may no longer be needed.

GrahamcOfBorg · 2018-05-07T20:58:54Z

Failure on x86_64-darwin (full log)

Attempted: stdenv

Partial log (click to expand)

cannot build derivation '/nix/store/190nn5mlssm8z0jvclig2a2621smbcw2-cctools-port-895.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/wnsn4fjdq23bymqxwrp9prkys51wfrm2-hook.drv': 6 dependencies couldn't be built
cannot build derivation '/nix/store/48kf92ls06827dcmni2adsiy7bbmyid9-ICU-osx-10.10.5.drv': 3 dependencies couldn't be built
cannot build derivation '/nix/store/zfkxsb39m9fcbqni4s6q3a6g6rms6dav-cctools-binutils-darwin.drv': 4 dependencies couldn't be built
cannot build derivation '/nix/store/ag6s2phnm5j915fpb0nmllj6j1wg2bxb-gnutar-1.30.drv': 3 dependencies couldn't be built
cannot build derivation '/nix/store/h78rrv2yn2f8b3956adg1bzspsa2ka1q-CF-osx-10.10.5.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/lz3bhkvdyjf35l798cdinnp9zl7dwmw9-cctools-binutils-darwin-wrapper.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/9bxrk07qamgjzzfql9zpkgvpfijza43f-clang-wrapper-5.0.2.drv': 9 dependencies couldn't be built
cannot build derivation '/nix/store/vjlrnb63rmf4x97c90k79h43vj97cmfz-stdenv-darwin.drv': 35 dependencies couldn't be built
error: build of '/nix/store/vjlrnb63rmf4x97c90k79h43vj97cmfz-stdenv-darwin.drv' failed

GrahamcOfBorg · 2018-05-07T21:00:38Z

Success on aarch64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

shrinking /nix/store/zic2s2k09xbp2vwc0z6xqh5gpsgqcsq2-findutils-4.6.0/bin/find
gzipping man pages under /nix/store/zic2s2k09xbp2vwc0z6xqh5gpsgqcsq2-findutils-4.6.0/share/man/
strip is /nix/store/gfgczbs0cy0blibb0acv39cayq7qbplg-bootstrap-tools/bin/strip
stripping (with command strip and flags -S) in /nix/store/zic2s2k09xbp2vwc0z6xqh5gpsgqcsq2-findutils-4.6.0/libexec  /nix/store/zic2s2k09xbp2vwc0z6xqh5gpsgqcsq2-findutils-4.6.0/bin
checking for references to /build in /nix/store/zic2s2k09xbp2vwc0z6xqh5gpsgqcsq2-findutils-4.6.0...
shrinking RPATHs of ELF executables and libraries in /nix/store/13xa2x7bsnbbh5fm29g596fl6ymqal0p-findutils-4.6.0-info
strip is /nix/store/gfgczbs0cy0blibb0acv39cayq7qbplg-bootstrap-tools/bin/strip
checking for references to /build in /nix/store/13xa2x7bsnbbh5fm29g596fl6ymqal0p-findutils-4.6.0-info...
building '/nix/store/cdv0qphnqinlmplf1p6hsg4rqlslximv-stdenv-linux.drv'...
/nix/store/wbkfacq1s26d6n7a9n0s073ifwbjd4i8-stdenv-linux

matthewbauer · 2018-05-23T18:06:53Z

@Ericson2314
Just a heads up but this is giving an issue in staging:

https://hydra.nixos.org/build/74502711/nixlog/1

/nix/store/9i0zrfqzfcx639s20x95r10fv9j7v6id-libiconv-osx-10.11.6/nix-support/setup-hook: line 5: getHostRole: command not found
builder for '/nix/store/rcfli01dwjbzvbwhw5k1lpkwzh57rqky-cairo-1.15.10.drv' failed with exit code 127

Not sure if it's since been fixed or not.

Fixes issue from PR #40139.

matthewbauer · 2018-05-23T18:10:15Z

Hopefully fixed in 89e196d.

Profpatsch · 2018-12-10T09:55:48Z

pkgs/stdenv/generic/setup.sh

                return 1
                ;;
        esac

-        content="${content//"$pattern"/$replacement}"
+        eval "$var"'=${'"$var"'//"$pattern"/"$replacement"}'


What’s this commit do? Such major changes to the internals of stdenv should at least have an extensive commit message of all the implications. I’m told substituteInPlace is very slow (in the order of seconds), so on inquiring this is the last commit that popped up, with no commit description whatsoever. :(

Maybe it was slow before, but evaling is a strong contender. This line should have a comment why it is necessary.

Was this resolved? Does this still have a performance problem? (If so, is there a known way to reproduce it?)

To reproduce it, try changing something in pkgs/development/tools/build-managers/bazel/default.nix and then run nix-build -A bazel /path/to/nixpkgs.

You should see that the substitution of every file takes like half a second, which is a lot for a simple text substitution.

See also #35304 (comment), cc @mboes

It's always been O(size of file * number of @var@ definitions), which is quiet shitty.

Just as we did for "expand response params", we should probably rewrite this in C or something.

dtzWill · 2019-01-03T09:05:21Z

I'm not seeing 0.5s locally, but it sure does add up! As a workaround for this particular case, I looked into it a bit and put together this workaround: dtzWill@9ed2c64 LMK if this looks good (dunno if it's any trouble to try it), if so we can submit a PR with it.

…

On Wed, 02 Jan 2019 08:15:20 -0800, Profpatsch ***@***.***> wrote: Profpatsch commented on this pull request. > return 1 ;; esac - content="${content//"$pattern"/$replacement}" + eval "$var"'=${'"$var"'//"$pattern"/"$replacement"}' To reproduce it, try changing something in `pkgs/development/tools/build-managers/bazel/default.nix` and then run `nix-build -A bazel /path/to/nixpkgs`. You should see that the substitution of every file takes like half a second, which is a lot for a simple text substitution. See also #35304 (comment), cc @mboes -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #40139 (comment) part: text/html

stdenv: Support concatenating setup hooks from multiple parts.

34a3233

Ericson2314 requested review from matthewbauer, orivej and dtzWill May 7, 2018 18:51

Ericson2314 mentioned this pull request May 7, 2018

Setup hooks are polluting NIX_LDFLAGS #40046

Closed

GrahamcOfBorg added the 6.topic: stdenv Standard environment label May 7, 2018

matthewbauer reviewed May 7, 2018

View reviewed changes

Ericson2314 added 2 commits May 7, 2018 15:10

treewide: Use pkgs/build-support/roles.bash to remove copy pasta

2110c0b

Also fix some setup hooks that unnecessarily used environment hooks, which revolted in the same variable being modified too many times.

{bintools,cc}-wrapper: Factor out role accumulation logic

8b0fce8

Ericson2314 force-pushed the modular-setup-hooks branch from ba95564 to 8b0fce8 Compare May 7, 2018 19:11

GrahamcOfBorg added 10.rebuild-darwin-stdenv 10.rebuild-linux-stdenv 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ labels May 7, 2018

matthewbauer reviewed May 7, 2018

View reviewed changes

Ericson2314 merged commit 4f7cdd3 into NixOS:staging May 7, 2018

Ericson2314 deleted the modular-setup-hooks branch May 7, 2018 19:32

hedning mentioned this pull request May 16, 2018

inkscape fails on nixpkgs-unstable #40586

Closed

matthewbauer added a commit that referenced this pull request May 23, 2018

libiconv: fix setup hook

89e196d

Fixes issue from PR #40139.

wizeman mentioned this pull request Nov 20, 2018

cantarell-fonts build broken (GNOME 3 dependency) #50855

Closed

Profpatsch reviewed Dec 10, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: Modular setup hooks #40139

treewide: Modular setup hooks #40139

Ericson2314 commented May 7, 2018 •

edited

matthewbauer May 7, 2018

Ericson2314 May 7, 2018

matthewbauer May 7, 2018

Ericson2314 May 7, 2018 •

edited

matthewbauer commented May 7, 2018

Ericson2314 commented May 7, 2018

matthewbauer May 7, 2018

Ericson2314 May 7, 2018 •

edited

matthewbauer commented May 7, 2018

GrahamcOfBorg commented May 7, 2018

matthewbauer commented May 7, 2018

GrahamcOfBorg commented May 7, 2018

GrahamcOfBorg commented May 7, 2018

matthewbauer commented May 23, 2018

matthewbauer commented May 23, 2018

Profpatsch Dec 10, 2018

Profpatsch Dec 10, 2018

dtzWill Dec 30, 2018

Profpatsch Jan 2, 2019

Ericson2314 Jan 2, 2019

dtzWill commented Jan 3, 2019 via email

treewide: Modular setup hooks #40139

treewide: Modular setup hooks #40139

Conversation

Ericson2314 commented May 7, 2018 • edited

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 May 7, 2018 • edited

Choose a reason for hiding this comment

matthewbauer commented May 7, 2018

Ericson2314 commented May 7, 2018

Choose a reason for hiding this comment

Ericson2314 May 7, 2018 • edited

Choose a reason for hiding this comment

matthewbauer commented May 7, 2018

GrahamcOfBorg commented May 7, 2018

matthewbauer commented May 7, 2018

GrahamcOfBorg commented May 7, 2018

GrahamcOfBorg commented May 7, 2018

matthewbauer commented May 23, 2018

matthewbauer commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzWill commented Jan 3, 2019 via email

Ericson2314 commented May 7, 2018 •

edited

Ericson2314 May 7, 2018 •

edited

Ericson2314 May 7, 2018 •

edited