bfd, opcodes: Init separate derivations for binutils libraries #30484

Ericson2314 · 2017-10-16T21:41:15Z

Motivation for this change

This is intended to be reviewed commit by commit.

On most distros, these are just built and distributed as part of binutils. We don't use binutils across the board, however, but rather switch between binutils and a cctools-binutils mashup, and change the
outputs on binutils too. This creates a combinatorial conditional soup which is hard to maintain.

My hope is to lower the the state space. While my patch isn't the most maintainable, they make downstream packages become more maintainable to compensate. The additional derivations themselves are completely platform-agnostic, always they always supports all possible target
platforms, and always yield "out" and "dev" outputs. That, in turn, allows downstream packages to not worry about a dependency shape-shifting under them.

In fact, the actual binutils package can avoid needing multiple outputs now that these serve the requisite libraries, so that also can become simpler on all platforms, too, removing the original wart this PR circumnavigates for now. Actually changing the binutils package to leverage is a mass rebuild, however, so I'll leave that for a separate PR.

I do hope to upstream something like my patch too, but until then I'll make myself maintainer of these derivations.

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
- 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.

CC @bgamari @orivej

Ericson2314 · 2017-10-16T21:47:58Z

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

+    # providing the linker and related tools. The two we use now are GNU
+    # Binutils, and Apple's "cctools"; "binutils" as an attempt to find an
+    # unused middle-ground name that evokes both.
+    bintools = binutils_bin;


@copumpkin I ended up going with "bintools" rather than "utils", for the reasons above, but the concept is the same as what I mentioned in #29396 (comment) .

Ericson2314 · 2017-10-16T21:48:39Z

pkgs/development/libraries/lightning/default.nix

@@ -1,4 +1,4 @@
-{ stdenv, fetchurl, binutils }:
+{ stdenv, fetchurl, libbfd, binutilsOpcodes }:


don't need libbfd here, oops

Ericson2314 · 2017-10-16T21:49:33Z

pkgs/top-level/all-packages.nix

@@ -7796,6 +7800,10 @@ with pkgs;

  belle-sip = callPackage ../development/libraries/belle-sip { };

+  libbfd = callPackage ../development/libraries/libbfd { };


bfd? libbfd? binutils-bfd? Help me bikeshed!

Maybe rename binutils-opcodes to libopcodes for consistency with libbfd? It seems to be known under this name, e.g. in http://mkfs.github.io/ and https://pypi.python.org/pypi/pybfd

Ah, good precedence finding!

Ericson2314 · 2017-10-16T21:51:30Z

pkgs/top-level/all-packages.nix

@@ -7796,6 +7800,10 @@ with pkgs;

  belle-sip = callPackage ../development/libraries/belle-sip { };

+  libbfd = callPackage ../development/libraries/libbfd { };
+
+  binutilsOpcodes = callPackage ../development/libraries/binutils-opcodes { };


opcodes? libopcodes? binutils-opcodes? Help me bikeshed!

I vote binutils-opcodes!

Yes, binutils-opcodes. We don't do camelCase in package names anymore.

We don't do camelCase in package names anymore.

Oh, glad I asked! I saw a vague trend but had no idea that was the policy. Kebob FTW!

Ericson2314 · 2017-10-16T21:51:52Z

pkgs/development/libraries/binutils-opcodes/opcodes-bfd_stdint.patch

@@ -0,0 +1,13 @@
+--- a/opcodes/configure.ac


Oops. this file can go.

abbradar

I'm not familiar with the problem; why do we have those conditional outputs in binutils in the first place?

abbradar · 2017-10-16T21:56:38Z

pkgs/top-level/all-packages.nix

@@ -7796,6 +7800,10 @@ with pkgs;

  belle-sip = callPackage ../development/libraries/belle-sip { };

+  libbfd = callPackage ../development/libraries/libbfd { };
+
+  binutilsOpcodes = callPackage ../development/libraries/binutils-opcodes { };


I vote binutils-opcodes!

Ericson2314 · 2017-10-16T22:14:04Z

@abbradar Some of the Darwin ones might just be because nobody bothered to muck through the allowedRequisites, but the cross ones are because of real build systems bugs, it seems (I got failures once trying to make them unconditional).

abbradar · 2017-10-16T22:26:38Z

Good, 👍 then! Conditional outputs are not something nice to deal with.

edolstra · 2017-10-17T09:51:14Z

In principle, building the libraries separately is nice, but: 1) The patch will probably make maintaining binutils significantly harder. 2) Applying the patch to binutils would cause a dependency on autoconf/automake during early stdenv bootstrap. 3) I don't really understand the motivation. What is the "combinatorial conditional soup"?

Ericson2314 · 2017-10-17T13:01:05Z

Applying the patch to binutils would cause a dependency on autoconf/automake during early stdenv bootstrap.

Right now, binutils itself is built the same as before---stock, rebuilding those libs. This is also gross, but means stdenv avoids those issues. If I successfully upstream the patch or something like it, then we won't need to regenerate anything avoiding the dep. That would be a good time to make the binutils and gdb use these.

The patch will probably make maintaining binutils significantly harder.

That's why I'm committing to maintain these new derivations and upstreaming the patch. If I suddenly go missing, well, one can revert and alias the new derivations to binutils. That's the best insurance I can offer :).

I don't really understand the motivation. What is the "combinatorial conditional soup"?

The multiple outputs mess: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/misc/binutils/default.nix#L53-L56 . Do note I want to soon™ get Linux<->Darwin cross compiling working; I fear even more logic will be needed for the outputs for that.

If binutils is just for the binaries, we can scrap all those and just do "out". I'd statically link to prevent derivations using libs from binutils, and then switch back to dynamic linking once the patch is upstreamed and binutils can depend on these new derivations.
binutils vs binutils-raw: https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/all-packages.nix#L6930-L6933 We don't fake gcc, for example, on Darwin, so this strikes me as unnecessarily incongruous and confusing.

That binutils executables are usually wrapped in cc-wrapper makes it all the more so. That's a lot of indirection to wrap one's mind around on Darwin!

edolstra · 2017-10-17T16:26:03Z

@Ericson2314 Thanks! Regarding

Right now, binutils itself is built the same as before

we would want to have binutils use the separate libraries, because otherwise we end up duplicating them. Which could lead to confusion if (say) both binutils and libbfd are in the build inputs.

Ericson2314 · 2017-10-17T16:31:40Z

@edolstra Sure, I'll look into doing that as part of this then. Thanks!

peti

The changes to ghc make sense to me within the rationale of this patch.

Ericson2314 · 2017-10-18T18:30:19Z

Based on that @edolstra said, this will have to go to staging instead. I made #30549 for the master-ready parts.

Ericson2314 · 2017-11-13T13:47:04Z

Ah, thanks. That's because the dev output is not the default for those antiquotations. Should be fixed now. @nixborg build

Ericson2314 · 2017-11-13T14:01:13Z

@nixborg build

nixborg · 2017-11-13T14:04:50Z

Jobset created at https://hydra.mayflower.de/jobset/nixos/pr-30484

Ericson2314 · 2017-11-14T16:57:45Z

@nixborg build

nixborg · 2017-11-14T17:00:45Z

Jobset created at https://hydra.mayflower.de/jobset/nixos/pr-30484

Ericson2314 · 2017-11-14T17:10:33Z

I just added a commit to make myself a binutils maintainer too, since we'll eventually want binutils to use the libbfd and libopcodes derivations, which will require coordination with upstream just like merging the patch the build those separately in the first place.

That doesn't change any hashes, so my old build still stands (I though the nixborg build would be a noop but staging changed in the interum). Merging, then.

orivej · 2017-11-18T21:31:07Z

This has broken rustc tests for Linux on staging. The first failed build: https://hydra.nixos.org/build/64149296 .

The test run-make/atomic-lock-free runs rustc with --target= i686-unknown-linux-gnu, x86_64-unknown-linux-gnu, arm-unknown-linux-gnueabi and runs nm for each output. The expected output looks like this:

atomic_lock_free.0.o:
00000000 T _ZN16atomic_lock_free10atomic_i1617h2e4fcd96500dd9adE
00000000 T _ZN16atomic_lock_free10atomic_i3217hf3342aa248394428E
…
nm: rust.metadata.bin: File format not recognized
nm: atomic_lock_free.0.bytecode.deflate: File format not recognized

i686 and x86_64 output is expected, but arm output is different and it fails the test:

nm: atomic_lock_free.0.o: File format is ambiguous
nm: Matching formats: elf32-littlearm elf32-littlearm-symbian elf32-littlearm-vxworks
nm: rust.metadata.bin: File format not recognized
nm: atomic_lock_free.0.bytecode.deflate: File format not recognized

/cc @vcunat

orivej · 2017-11-19T01:27:58Z

Here is the object file produced by rust that the staging nm fails to decode: atomic_lock_free.0.o.gz

$ gunzip atomic_lock_free.0.o.gz
$ nm atomic_lock_free.0.o
nm: atomic_lock_free.0.o: File format is ambiguous
nm: Matching formats: elf32-littlearm elf32-littlearm-symbian elf32-littlearm-vxworks

It works with the master nm or when the staging nm is called with nm --target=elf32-little.

orivej · 2017-11-19T01:57:17Z

I'm going to apply this patch: https://sourceware.org/ml/binutils/2013-05/msg00271.html

#30484 (comment) Since [1] libbfd is compiled with support for all available targets. However, it can not choose whether an ARM ELF file is elf32-littlearm, elf32-littlearm-symbian, or elf32-littlearm-vxworks, and fails with the "File format is ambiguous" error. Here [2] Alan Modra intended to prioritize the first of the three, but although his patch was merged and reportedly solved the issue, currently glibc 2.28.1 and 2.29.1 again fail to disambiguate these targets. This commit makes it prioritize elf32-littlearm over the other two. [1] f8741c3 [2] https://sourceware.org/ml/binutils/2013-05/msg00271.html

orivej · 2017-11-19T03:58:51Z

Actually that patch was merged and reportedly solved the issue. Glibc must have regressed since then. I've commited the following patch to staging: c76890f . It is worth fixing the issue upstream (I've confirmed that 2.29.1 is affected too) and my patch is not worth upstreaming as it is, but it is safe and it works.

orivej · 2017-11-19T04:28:03Z

Reported upstream: https://sourceware.org/bugzilla/show_bug.cgi?id=22458

orivej · 2017-11-19T08:52:29Z

pybfd also needs fixing on staging: #31818
First failed build: https://hydra.nixos.org/build/64179131

PR NixOS#30484 (f8741c3) has split libbfd and libopcodes from binutils and gdb. The original pybfd setup.py is completely unsuitable to handle that. This commit replaces the original source with a fork with a patched setup.py.

Ericson2314 · 2017-11-20T18:42:47Z

@orivej This is extremely thorough troubleshooting and patching. Thank you so much!

orivej · 2017-11-21T02:48:15Z

@Ericson2314 You are welcome!

After this PR (not in master yet), binutils ship windres utility. These packages attempt to use it and fail to build:

#30484 (comment)

orivej · 2017-11-21T04:31:44Z

I've fixed pdf2djvu in 0b9cd7a and forced the others to expect that windres is missing without testing in 4f73b31. (Feel free to fix the latter without setting internal autoconf variables if you want to.)

Ericson2314 · 2017-11-21T15:13:56Z

My ultimate plan is single binutils always (we still pass --target for the moment) and arch-specific bintools-wrapper. Maybe we should just not define WINDRES outside of windows, then.

orivej · 2017-11-21T15:17:57Z

Only pdf2djvu had correct logic that was overridden by the presence of WINDRES environment variable; other projects just assumed that if windres is in PATH they are on Windows. Since this affects just 5 projects, let's keep it as it is.

Ericson2314 · 2017-11-21T15:20:02Z

Ah ok, I misunderstood. Good choice then.

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

Ericson2314 requested review from copumpkin, LnL7, abbradar and vcunat October 16, 2017 21:41

Ericson2314 requested a review from edolstra as a code owner October 16, 2017 21:41

Ericson2314 commented Oct 16, 2017

View reviewed changes

Ericson2314 changed the title ~~bfd, opcodes: Init separate derivations for binutils libraries~~ bfd, opcodes: WIP Init separate derivations for binutils libraries Oct 16, 2017

abbradar reviewed Oct 16, 2017

View reviewed changes

Ericson2314 force-pushed the libbfd branch from 9d6bc74 to 3f3af83 Compare October 16, 2017 22:11

Ericson2314 force-pushed the libbfd branch 2 times, most recently from f068043 to b3e68a9 Compare October 17, 2017 00:46

Ericson2314 force-pushed the libbfd branch from b3e68a9 to f417e82 Compare October 17, 2017 16:08

Ericson2314 force-pushed the libbfd branch from f417e82 to b473b75 Compare October 18, 2017 18:07

Ericson2314 requested a review from peti as a code owner October 18, 2017 18:07

Ericson2314 force-pushed the libbfd branch 2 times, most recently from 2ee0ab9 to 9594ac9 Compare October 18, 2017 18:25

peti reviewed Oct 18, 2017

View reviewed changes

Ericson2314 mentioned this pull request Oct 18, 2017

treewide: Introduce stdenv.cc.bintools #30549

Merged

8 tasks

Ericson2314 changed the base branch from master to staging October 18, 2017 18:30

Ericson2314 force-pushed the libbfd branch 3 times, most recently from af3ce14 to 88e960c Compare October 18, 2017 18:54

Ericson2314 force-pushed the libbfd branch from 67ff1d7 to ff023c9 Compare November 13, 2017 13:46

binutils: Make myself a maintainer

eaacf47

Ericson2314 merged commit 728446f into NixOS:staging Nov 14, 2017

Ericson2314 deleted the libbfd branch November 14, 2017 17:10

orivej mentioned this pull request Nov 19, 2017

python.pkgs.pybfd: support split libbfd and libopcodes #31818

Merged

8 tasks

orivej added a commit that referenced this pull request Nov 21, 2017

pdf2djvu: ignore windres on linux to fix build

0b9cd7a

#30484 (comment)

orivej added a commit that referenced this pull request Nov 21, 2017

treewide: preset windres missing to fix builds on Linux

4f73b31

#30484 (comment)

orivej mentioned this pull request Dec 31, 2017

binutils: Fix cross, again #33186

Merged

8 tasks

		@@ -1,4 +1,4 @@
		{ stdenv, fetchurl, binutils }:
		{ stdenv, fetchurl, libbfd, binutilsOpcodes }:

		@@ -7796,6 +7800,10 @@ with pkgs;

		belle-sip = callPackage ../development/libraries/belle-sip { };

		libbfd = callPackage ../development/libraries/libbfd { };

bfd, opcodes: Init separate derivations for binutils libraries #30484

bfd, opcodes: Init separate derivations for binutils libraries #30484

Conversation

Ericson2314 commented Oct 16, 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

orivej Nov 10, 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

abbradar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Oct 16, 2017

abbradar commented Oct 16, 2017

edolstra commented Oct 17, 2017

Ericson2314 commented Oct 17, 2017 • edited

edolstra commented Oct 17, 2017

Ericson2314 commented Oct 17, 2017 • edited

peti left a comment

Choose a reason for hiding this comment

Ericson2314 commented Oct 18, 2017

Ericson2314 commented Nov 13, 2017

Ericson2314 commented Nov 13, 2017

nixborg commented Nov 13, 2017

Ericson2314 commented Nov 14, 2017

nixborg commented Nov 14, 2017

Ericson2314 commented Nov 14, 2017

orivej commented Nov 18, 2017 • edited

orivej commented Nov 19, 2017 • edited

orivej commented Nov 19, 2017

orivej commented Nov 19, 2017

orivej commented Nov 19, 2017

orivej commented Nov 19, 2017

Ericson2314 commented Nov 20, 2017

orivej commented Nov 21, 2017 • edited

orivej commented Nov 21, 2017

Ericson2314 commented Nov 21, 2017

orivej commented Nov 21, 2017

Ericson2314 commented Nov 21, 2017

Ericson2314 commented Oct 16, 2017 •

edited

orivej Nov 10, 2017 •

edited

Ericson2314 commented Oct 17, 2017 •

edited

Ericson2314 commented Oct 17, 2017 •

edited

orivej commented Nov 18, 2017 •

edited

orivej commented Nov 19, 2017 •

edited

orivej commented Nov 21, 2017 •

edited