Skip to content

binutils-unwrapped: expose if built with ld.gold #132538

Merged
Ericson2314 merged 2 commits intoNixOS:stagingfrom
sternenseemann:has-gold
Aug 21, 2021
Merged

binutils-unwrapped: expose if built with ld.gold #132538
Ericson2314 merged 2 commits intoNixOS:stagingfrom
sternenseemann:has-gold

Conversation

@sternenseemann
Copy link
Copy Markdown
Member

@sternenseemann sternenseemann commented Aug 3, 2021

Motivation for this change

ld.gold is “A new, faster, ELF only linker”. Thus we only should pass
the configure flag --with-gold if our target platform will actually
support gold (in which case binutil's configure script silently
disables it).

With this change, not only will configureFlags represent the actual
configuration more closely, but we can also expose if the binutils
derivation contains ld.gold via a passthru attr. Specifically this
means that:

nix-repl> pkgsCross.mingwW64.stdenv.cc.bintools.bintools.hasGold
false

The intended way to use this is to check
stdenv.cc.bintools.bintools or false which returns accurate results
regardless of the actual linker derivation.

TODO: maybe also add hasGold to binutils wrapper as it also symlinks
ld.gold in?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Aug 3, 2021
@sternenseemann sternenseemann mentioned this pull request Aug 3, 2021
Merged
11 tasks
@ofborg ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. label Aug 3, 2021
@ofborg ofborg bot requested review from MarcWeber, kosmikus and peti August 3, 2021 11:46
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Aug 3, 2021
@r-rmcgibbo
Copy link
Copy Markdown

r-rmcgibbo commented Aug 3, 2021 •
edited
Loading

Result of nixpkgs-review pr 132538 at c664979c run on aarch64-linux 1

2 packages skipped due to time constraints:
  • simavr
  • tinygo
19 packages built successfully:
  • armTrustedFirmwareAllwinner
  • armTrustedFirmwareQemu
  • armTrustedFirmwareRK3328
  • armTrustedFirmwareRK3399
  • armTrustedFirmwareS905
  • gnuk
  • ubootBananaPim64
  • ubootNanoPCT4
  • ubootOdroidC2
  • ubootOrangePiZeroPlus2H5
  • ubootPine64
  • ubootPine64LTS
  • ubootPinebook
  • ubootPinebookPro
  • ubootROCPCRK3399
  • ubootRock64
  • ubootRockPi4
  • ubootRockPro64
  • ubootSopine

Result of nixpkgs-review pr 132538 at c664979c run on x86_64-linux 1

2 packages failed to build:
20 packages skipped due to time constraints:
  • grapejuice
  • lutris
  • lutris-free
  • lutris-unwrapped
  • pipelight
  • playonlinux
  • simavr
  • tinygo
  • wine (winePackages.full)
  • wine-staging
  • ...
4 packages built successfully:
  • armTrustedFirmwareTools
  • axoloti
  • fusee-launcher
  • gnuk

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@alexfmpe
Copy link
Copy Markdown
Member

alexfmpe commented Aug 3, 2021

I dunno what's idiomatic with passthru and all that, but this is much better than the special-casing I was doing for gold on windows in #132199.

@alexfmpe
Copy link
Copy Markdown
Member

alexfmpe commented Aug 3, 2021

Do 8.6.5-binary.nix & 8.10.2-binary.nix not need this?

@sternenseemann
Copy link
Copy Markdown
Member Author

sternenseemann commented Aug 3, 2021

Do 8.6.5-binary.nix & 8.10.2-binary.nix not need this?

No, I don't think we can influence the linker they use anyways. I assume they always call plain ld.

@sternenseemann sternenseemann changed the base branch from master to staging August 3, 2021 21:51
@sternenseemann
Copy link
Copy Markdown
Member Author

sternenseemann commented Aug 3, 2021

What uses GNU binutils on darwin? These rebuilds confuse me a bit, seems like we need to do some testing there.

@alexfmpe
Copy link
Copy Markdown
Member

alexfmpe commented Aug 4, 2021

No, I don't think we can influence the linker they use anyways.

Yeah, I don’t see a gold flag or such.

I assume they always call plain ld.

Almost always, it seems.
https://github.com/NixOS/nixpkgs/blob/4643bd80bd4401046fcca0bc4e35f4c2e41082b4/pkgs/development/compilers/ghc/8.10.2-binary.nix#L240-L243

@sternenseemann
Copy link
Copy Markdown
Member Author

sternenseemann commented Aug 4, 2021

I'm pretty sure LD refers to the build linker here, maybe @expipiplus1 or @Ericson2314 can confirm this? What confuses me here is the use of targetPlatform, maybe it was a bit overeager?

Maybe we should be more eager to use gold here anyways, since it likely speeds up linking?!

Ericson2314
Ericson2314 reviewed Aug 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I would do gold ? execFormatIsELF and assert gold -> execFormatIsELF, as explicitly asking for gold when it won't build should be an error.

@sternenseemann
Copy link
Copy Markdown
Member Author

sternenseemann commented Aug 17, 2021 •
edited
Loading

Makes sense yeah, but I wouldn't remove the hasGold thing. Although it is unlikely to ever change that hasGold -> execFormatIsELF, it makes code easier to understand and requires less special knowledge. Not sure if you were requesting this change even though?!

@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented Aug 17, 2021

Yeah I would expose hasGold = gold in passthru but not bother let-binding it.

@sternenseemann
binutils-unwrapped: expose if built with ld.gold
4c75874 
ld.gold is “A new, faster, ELF only linker”. Thus we only should pass
the configure flag --with-gold if our target platform will actually
support gold (in which case binutil's configure script silently
disables it).

With this change, not only will configureFlags represent the actual
configuration more closely, but we can also expose if the binutils
derivation contains ld.gold via a passthru attr. Specifically this
means that:

    nix-repl> pkgsCross.mingwW64.stdenv.cc.bintools.bintools.hasGold
    false

The intended way to use this is to check
`stdenv.cc.bintools.bintools or false` which returns accurate results
regardless of the actual linker derivation.

TODO: maybe also add hasGold to binutils wrapper as it also symlinks
ld.gold in?
@sternenseemann
haskell.compiler.*: check bintools.hasGold before enabling ld.gold
0908812
@sternenseemann
Copy link
Copy Markdown
Member Author

sternenseemann commented Aug 17, 2021

Done in 4c75874.

@Ericson2314 Ericson2314 merged commit 7195e60 into NixOS:staging Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Ericson2314 left review comments

@alexfmpe alexfmpe Awaiting requested review from alexfmpe

@alyssais alyssais Awaiting requested review from alyssais

@cdepillabout cdepillabout Awaiting requested review from cdepillabout

@expipiplus1 expipiplus1 Awaiting requested review from expipiplus1

@maralorn maralorn Awaiting requested review from maralorn

@peti peti Awaiting requested review from peti

@kosmikus kosmikus Awaiting requested review from kosmikus

@MarcWeber MarcWeber Awaiting requested review from MarcWeber

Assignees

No one assigned

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sternenseemann @r-rmcgibbo @alexfmpe @Ericson2314