Bintools Wrapper: Init by refactoring out of cc-wrapper, take 2 by Ericson2314 · Pull Request #29396 · NixOS/nixpkgs

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

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

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

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

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

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

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

"Sine" is a typo.

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

doc/stdenv.xml

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

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

doc/stdenv.xml

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

"the" should be "that"

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

doc/stdenv.xml

"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 LnL7 and copumpkin 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 platform than they will be used on label Sep 20, 2017

Ericson2314 added this to the 17.09 milestone Sep 20, 2017

DavidEGrayson reviewed Sep 20, 2017

View reviewed changes

doc/stdenv.xml Outdated

Copy link

Contributor

DavidEGrayson Sep 20, 2017

Choose a reason for hiding this comment

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

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

DavidEGrayson reviewed Sep 20, 2017

View reviewed changes

doc/stdenv.xml Outdated

Copy link

Contributor

DavidEGrayson Sep 20, 2017

Choose a reason for hiding this comment

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

"the" should be "that"

DavidEGrayson reviewed Sep 20, 2017

View reviewed changes

doc/stdenv.xml Outdated

Copy link

Contributor

DavidEGrayson Sep 20, 2017

Choose a reason for hiding this comment

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

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

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 force-pushed the binutils-wrapper branch from 49e0ad4 to ce46fed Compare September 20, 2017 23:25

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

Uh oh!

Conversation

Ericson2314 commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orivej Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Dec 13, 2017

Uh oh!

orivej commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orivej commented Dec 14, 2017

Uh oh!

orivej commented Dec 19, 2017

Uh oh!

Ericson2314 commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orivej commented Dec 19, 2017

Uh oh!

7c6f434c commented Dec 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Ericson2314 commented Sep 14, 2017 •

edited

Loading

orivej Sep 16, 2017 •

edited

Loading

orivej commented Dec 14, 2017 •

edited

Loading

Ericson2314 commented Dec 19, 2017 •

edited

Loading