Bintools Wrapper: Init by refactoring out of cc-wrapper, take 2 #29396

Ericson2314 · 2017-09-14T20:59:22Z

edit see my question in #29396 (comment)

Motivation for this change

This is meant to be reviewed commit by commit.

This is needed so packages that just use binutils get the same setup-hook support. That is, variables like BUILD_AS or LD should be defined the same way, along with the LD_FLAGS. This will be necessary to clean up at least the gcc derivation (which must be built with an binutils not associated with any existsing C compiler when target != host), and probably other compilers too.

More broadly, having two wrappers will help enforce proper layering between binutils and the C compiler. For languages besides C/C++, it would be neat to use `stdenvNoCC + binutils, avoiding a C compiler dependency. This wrapper makes that change possible.

@edolstra I'm hoping for you to merge this change when you approve of it, to avoid a repeat of last time.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built stdenv on platform(s)
- NixOS --- in progress
- macOS
- Linux
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 · 2017-09-14T21:00:46Z

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

+      # TODO(@Ericson2314): Remove this after stable release and force
+      # everyone to refer to binutils-wrapper directly.
+      if [[ -f "$binutils/nix-support/dynamic-linker" ]]; then
+        ln -s "$binutils/nix-support/dynamic-linker" "$out/nix-support"


@edolstra this symlink avoids the incompatibility the old version introduced.

orivej · 2017-09-15T00:30:35Z

pkgs/build-support/cc-wrapper/ld-wrapper.sh

+# Choose 32-bit dynamic linker if needed
+if [ -e @out@/nix-support/dynamic-linker-m32 ]; then
+    prev=
+    for p in ${params+"${params[@]}"}; do


It was determined to be valuable not to iterate over params more times than necessary. Could you merge this loop into the next one? (It has already merged two cases, [ "$NIX_@infixSalt@_DONT_SET_RPATH" != 1 ] and [ "$NIX_@infixSalt@_SET_BUILD_ID" = 1 ].)

orivej · 2017-09-16T05:06:48Z

pkgs/build-support/binutils-wrapper/ld-wrapper.sh

-if [ "$NIX_@infixSalt@_DONT_SET_RPATH" != 1 ] || [ "$NIX_@infixSalt@_SET_BUILD_ID" = 1 ]; then
+# Two tasks:
+#
+#   1. Find all -L... switches for rpath, and relocatable flags for build id.


These cases are not related, so if you enumerate the cases, these should be 1 and 2.

True. Good catch.

orivej · 2017-09-16T05:18:49Z

pkgs/build-support/binutils-wrapper/setup-hook.sh

+    case $depOffset in
+        -1) local role='BUILD_' ;;
+        0)  local role='' ;;
+        1)  local role='TARGET_' ;;


Where may depOffset be set to 1? I find only places where it is set to -1 or 0.

Oh heh. That would anticipate #26805

Cc-wrapper already has that, I guess I'll leave it as consistent anticipation.

DavidEGrayson · 2017-09-20T16:34:28Z

doc/stdenv.xml

-        CC wrapper's setup hook causes any <filename>include</filename> subdirectory of such a dependency to be added to <envar>NIX_CFLAGS_COMPILE</envar>, and any <filename>lib</filename> and <filename>lib64</filename> subdirectories to <envar>NIX_LDFLAGS</envar>.
-        The setup hook itself contains some lengthy comments describing the exact convoluted mechanism by which this is accomplished.
+        Binutils Wrapper's setup hook causes any <filename>lib</filename> and <filename>lib64</filename> subdirectories to <envar>NIX_LDFLAGS</envar>.
+        Sine CC Wrapper and Binutils Wrapper use the same strategy, most of the Binutils Wrapper code is sparsely commented and refers to CC Wrapper.


"Sine" is a typo.

DavidEGrayson · 2017-09-20T17:53:56Z

doc/stdenv.xml

        It is currently accomplished by collecting directories of host-platform dependencies (i.e. <varname>buildInputs</varname> and <varname>nativeBuildInputs</varname>) in environment variables.
-        CC wrapper's setup hook causes any <filename>include</filename> subdirectory of such a dependency to be added to <envar>NIX_CFLAGS_COMPILE</envar>, and any <filename>lib</filename> and <filename>lib64</filename> subdirectories to <envar>NIX_LDFLAGS</envar>.
-        The setup hook itself contains some lengthy comments describing the exact convoluted mechanism by which this is accomplished.
+        Binutils Wrapper's setup hook causes any <filename>lib</filename> and <filename>lib64</filename> subdirectories to <envar>NIX_LDFLAGS</envar>.


Probably you're missing the words "be added to" in this sentence.

DavidEGrayson · 2017-09-20T17:56:47Z

doc/stdenv.xml

        Firstly, this helps poorly-written packages, e.g. ones that look for just <command>gcc</command> when <envar>CC</envar> isn't defined yet <command>clang</command> is to be used.
-        Secondly, this helps packages not get confused when cross-compiling, in which case multiple CC wrappers may be simultaneous in use (targeting different platforms).
-        <envar>BUILD_</envar>- and <envar>TARGET_</envar>-prefixed versions of the normal environment variable are defined for the additional CC Wrappers, properly disambiguating them.
+        Secondly, this helps packages not get confused when cross-compiling, in which case multiple Binutils Wrappers may be simultaneous in use (targeting different platforms).


The idomatic wording options would be:

"may be in use simultaneously", or

"may be in simultaneous use"

DavidEGrayson · 2017-09-20T17:58:38Z

doc/stdenv.xml

+      </para>
+      <para>
+        Dependency finding is undoubtedly the main task of CC Wrapper.
+        This works just like Binutils Wrapper, except the any <filename>include</filename> subdirectory of any relevant dependency is added to <envar>NIX_CFLAGS_COMPILE</envar>.


"the" should be "that"

DavidEGrayson · 2017-09-20T17:59:50Z

doc/stdenv.xml

        Most packages, however, firstly use the C compiler for linking, secondly use <envar>LD</envar> anyways, defining it as the C compiler, and thirdly, only so define <envar>LD</envar> when it is undefined as a fallback.
-        This triple-threat means CC Wrapper will break those packages, as LD is already defined as the actually linker which the package won't override yet doesn't want to use.
+        This triple-threat means Binutils Wrapper will break those packages, as LD is already defined as the actually linker which the package won't override yet doesn't want to use.


"actually" is not an adjective, probably you meant "actual".

This avoids any `NIX_FOOBAR=1 1` not triggering conditions.

It means stdin, and is morally equivalent to passing a file. e.g. $ echo 'int main(void) { return 0; }' | gcc -x c - will compile and link a binary.

Factor a bintools (i.e. binutils / cctools) wrapper out of cc-wrapper. While only LD is wrapped, the setup hook defines environment variables on behalf of other utilites.

Shrunk the CC Wrapper documentation so as not to be repetative.

This is more robust for cross-compilation

Also make the code more precise in the process

These will be installed if the wrappers are. The wrappers aren't very good to install, but that's another matter.

Will need to be editted again to work for cross

…-wrapper

Ericson2314 · 2017-12-13T21:37:05Z

@edolstra told me he doesn't have time to review this any time soon, so we might as well not hold it up further. Yay, thanks!

This has passed nixborg CI many times before. the last change is a mass rebuild, but just from merging with staging. The post-merge change is just an eval fix for a newer package (clang multilib). Merging, but will keep an I on staging just in case.

orivej · 2017-12-14T00:54:21Z

This broke bloaty, fixed in fe61c3b.

orivej · 2017-12-14T01:24:52Z

This broke lispPackages.cl-fuse. @Ericson2314 could you fix it? It exports NIX_LDFLAGS with -L/nix/store/wr579906xik1qbnblm0wq1nissxh53sy-fuse-2.9.7/lib and runs gcc (wrapper), but gcc invokes unwrapped gcc without these NIX_LDFLAGS. Here is this sequence of steps: https://gist.github.com/orivej/4c8fc1b12d8d630edf24b6ff5f88cd80 (141-142-sbcl runs sbcl, which runs gcc in the same way as 142-143-gcc, which runs the unwrapped gcc just like 143-144-gcc .) Here is how NIX_LDFLAGS are defined:

nixpkgs/pkgs/development/lisp-modules/define-package.nix

Line 23 in ea15e43

    
               echo "export NIX_LDFLAGS='$NIX_LDFLAGS'\"\''${NIX_LDFLAGS:+ \$NIX_LDFLAGS}\"" >> "$config_script"

orivej · 2017-12-19T00:09:04Z

Two perl packages,perlPackages.EncodeDetect and perlPackages.ExtUtilsCppGuess, fail to build with ld: cannot find -lstdc++. They are needed for spamassassin and slic3r. @Ericson2314 agreed to try to fix them.

It seems that lispPackages.cl-fuse may not be fixed before the merge to master. /cc @7c6f434c

Ericson2314 · 2017-12-19T00:21:32Z

Can't fix this instant, but the crux of the Perl packages' problem is that ld no longer magically see's into the C(XX)'s compiler's lib directory. LD=$CC might be the best solution to avoid violating layers for now.

after NixOS#29396 removed `-L path/to/dir/of/libstdc++.so` from ld flags See NixOS#29396 (comment) Module::Build build helper works correctly when LD is unset (taking LD from Perl config to be `cc`). However, we can not unset LD because this goes contrary to the cross compilation effort, and we can not make it propagate ld-is-cc-hook because it breaks e.g. the build of `libguestfs`. However, NixOS#29396 makes LD=ld incompatible with just 3 perl packages; they are individually fixed by this commit.

orivej · 2017-12-19T02:28:10Z

Thanks! I'm fixing perl packages in #32830.

7c6f434c · 2017-12-19T16:41:23Z

Hm. I don't really understand where NIX_LDFLAGS gets lost in CL-Fuse build… So I gave up and the shared library is now prebuilt separately.

Ericson2314 assigned edolstra Sep 14, 2017

Ericson2314 requested a review from edolstra September 14, 2017 20:59

Ericson2314 requested review from copumpkin and LnL7 as code owners September 14, 2017 20:59

Ericson2314 commented Sep 14, 2017

View reviewed changes

Ericson2314 changed the title ~~Binutils wrapper: Init by refactoring out of cc-wrapper~~ Binutils wrapper: Init tak 2 by refactoring out of cc-wrapper Sep 14, 2017

Ericson2314 changed the title ~~Binutils wrapper: Init tak 2 by refactoring out of cc-wrapper~~ Binutils wrapper: Init by refactoring out of cc-wrapper, take 2 Sep 14, 2017

Ericson2314 added the 2.status: work-in-progress label Sep 14, 2017

Ericson2314 force-pushed the binutils-wrapper branch from d8c0c7e to d753d4c Compare September 14, 2017 23:04

Ericson2314 mentioned this pull request Sep 14, 2017

Binutils-wrapper: Init by refactoring out of cc-wrapper #28557

Merged

8 tasks

orivej reviewed Sep 15, 2017

View reviewed changes

Ericson2314 force-pushed the binutils-wrapper branch 2 times, most recently from beb7de1 to f1a1ac2 Compare September 15, 2017 17:16

orivej reviewed Sep 16, 2017

View reviewed changes

Ericson2314 force-pushed the binutils-wrapper branch from f1a1ac2 to d9a2380 Compare September 19, 2017 03:28

Ericson2314 mentioned this pull request Sep 19, 2017

cc-wrapper: Clean up dynamic linking with x86 multilib #29568

Merged

9 tasks

Ericson2314 force-pushed the binutils-wrapper branch 3 times, most recently from 3dbb0a3 to 621ae3d Compare September 19, 2017 23:36

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

Ericson2314 added this to the 17.09 milestone Sep 20, 2017

Ericson2314 added this to Needed by the big PR---nice to move pick off pieces of it and move here, rebasing the big PR on top in Cross compilation Sep 20, 2017

DavidEGrayson reviewed Sep 20, 2017

View reviewed changes

Ericson2314 force-pushed the binutils-wrapper branch from 621ae3d to 49e0ad4 Compare September 20, 2017 21:30

Ericson2314 requested a review from FRidh as a code owner September 20, 2017 21:30

Ericson2314 and others added 13 commits December 13, 2017 16:08

cc-wrapper: Use separate mangler for "bool" variables

bdd6c03

This avoids any `NIX_FOOBAR=1 1` not triggering conditions.

cc-wrapper: Don't treat "-" alone as a flag

4f869bc

It means stdin, and is morally equivalent to passing a file. e.g. $ echo 'int main(void) { return 0; }' | gcc -x c - will compile and link a binary.

bintools-wrapper: Init

8e557ed

Factor a bintools (i.e. binutils / cctools) wrapper out of cc-wrapper. While only LD is wrapped, the setup hook defines environment variables on behalf of other utilites.

bintools-wrapper: Import separately from cc-wrapper

2bba929

doc: Document Bintools Wrapper

91ca46f

Shrunk the CC Wrapper documentation so as not to be repetative.

misc setup-hooks: Use env vars to refer to binutils programs

b8a21aa

This is more robust for cross-compilation

bintools-wrapper: Support ld.ldd, along with ld.bfd and ld.gold

ef178be

Also make the code more precise in the process

darwin binutils: Better handling of man pages and info

cebe1b4

bintools-wrapper: Create man and info outputs propagated underlying ones

99806c5

These will be installed if the wrappers are. The wrappers aren't very good to install, but that's another matter.

gcc: Adjust builder.sh to find some things in bintools-wrapper instead

12e0672

libstdc++5: Fix after binutils-wrapper

26a5612

Will need to be editted again to work for cross

Merge commit '9d8f9b2e531bf95a700a949d879927fb6996ffc9' into binutils…

7ef4448

…-wrapper

clang multi: Fix post bintools wrapper

84fb59e

Ericson2314 force-pushed the binutils-wrapper branch from d816053 to 84fb59e Compare December 13, 2017 21:11

Merge remote-tracking branch 'upstream/staging' into binutils-wrapper

a0b1ebe

Ericson2314 merged commit c51f27d into NixOS:staging Dec 13, 2017

Ericson2314 deleted the binutils-wrapper branch December 13, 2017 21:37

orivej added a commit that referenced this pull request Dec 14, 2017

bloaty: update path to c++filt after #29396

fe61c3b

orivej mentioned this pull request Dec 19, 2017

Fix build of 3 perl packages with ld-is-cc-hook #32830

Merged

8 tasks

Ericson2314 mentioned this pull request Dec 30, 2017

doc: Breaking change release note for bintools-wrapper #33196

Merged

8 tasks

Ekleog mentioned this pull request Oct 29, 2018

Reconsider exporting LD, AS, STRIP etc. in non-cross builds #29846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bintools Wrapper: Init by refactoring out of cc-wrapper, take 2 #29396

Bintools Wrapper: Init by refactoring out of cc-wrapper, take 2 #29396

Ericson2314 commented Sep 14, 2017 •

edited

Ericson2314 Sep 14, 2017

orivej Sep 15, 2017

Ericson2314 Sep 15, 2017

Ericson2314 Sep 16, 2017

orivej Sep 16, 2017 •

edited

Ericson2314 Sep 16, 2017

Ericson2314 Sep 19, 2017

orivej Sep 16, 2017

Ericson2314 Sep 16, 2017

Ericson2314 Sep 20, 2017

DavidEGrayson Sep 20, 2017

Ericson2314 Sep 20, 2017

DavidEGrayson Sep 20, 2017

DavidEGrayson Sep 20, 2017

DavidEGrayson Sep 20, 2017

DavidEGrayson Sep 20, 2017

Ericson2314 commented Dec 13, 2017

orivej commented Dec 14, 2017 •

edited

orivej commented Dec 14, 2017

orivej commented Dec 19, 2017

Ericson2314 commented Dec 19, 2017 •

edited

orivej commented Dec 19, 2017

7c6f434c commented Dec 19, 2017

Bintools Wrapper: Init by refactoring out of cc-wrapper, take 2 #29396

Bintools Wrapper: Init by refactoring out of cc-wrapper, take 2 #29396

Conversation

Ericson2314 commented Sep 14, 2017 • 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

Choose a reason for hiding this comment

orivej Sep 16, 2017 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Dec 13, 2017

orivej commented Dec 14, 2017 • edited

orivej commented Dec 14, 2017

orivej commented Dec 19, 2017

Ericson2314 commented Dec 19, 2017 • edited

orivej commented Dec 19, 2017

7c6f434c commented Dec 19, 2017

Ericson2314 commented Sep 14, 2017 •

edited

orivej Sep 16, 2017 •

edited

orivej commented Dec 14, 2017 •

edited

Ericson2314 commented Dec 19, 2017 •

edited