Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 10 commits into from
Sep 3, 2017
Merged

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

merged 10 commits into from
Sep 3, 2017

Conversation

Ericson2314
Copy link
Member

Motivation for this change

I need this so packages that just use binutils (e.g. gcc building its standard library until #26004) get the same setup-hook support. More broadly, having two wrappers will help enforce proper layering between binutils and the C compiler.

This depends on #28556

Things done

Right now cc-wrapper just calls binutils wrapper, but I should add another commit exposing wrapped binutils on it's own and changing the stdenv bootstrapping.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • 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 Ericson2314 added 0.kind: enhancement 1.severity: mass-rebuild 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 6.topic: hygiene labels Aug 25, 2017
@Ericson2314 Ericson2314 added this to the 17.09 milestone Aug 25, 2017
@Ericson2314 Ericson2314 mentioned this pull request Aug 25, 2017
Merged
13 tasks
@Ericson2314 Ericson2314 force-pushed the binutils-wrapper branch 3 times, most recently from 97349ab to c10e6cf Compare August 26, 2017 21:57
@Ericson2314
Copy link
Member Author

Something really weird is going on where binutils' configure tests for various headers fail despite the underlying gcc -E conftest.c succeeding.

@Ericson2314 Ericson2314 force-pushed the binutils-wrapper branch 13 times, most recently from 063cd8c to a0acff6 Compare August 30, 2017 19:10
@Ericson2314
Copy link
Member Author

Ok the Darwin `allowedRequsites problem is from a self rpath, it seems:

    time stamp 2 Wed Dec 31 19:00:02 1969
       current version 1226.10.1
 compatibility version 1.0.0
 Load command 11
+          cmd LC_RPATH
+      cmdsize 88
+         path /nix/store/n3s943551i12fvklcxxs023bfy2izk8g-libresolv-osx-10.11.6/lib (offset 12)
+Load command 12
       cmd LC_FUNCTION_STARTS
   cmdsize 16
   dataoff 162176

@Ericson2314
cc-wrapper: Use separate mangler for "bool" variables
fbb7d33 
This avoids any `NIX_FOOBAR=1 1` not triggering conditions.
@Ericson2314
binutils-wrapper: Init
40e9b2a 
Factor a binutils wrapper out of cc-wrapper. While only LD is wrapped,
the setup hook defines environment variables on behalf of other
utilites.
@Ericson2314
misc pkgs: Use $NIX_BINUTILS for when needed
3f30cff 
Certain files are now only there instead of $NIX_CC (some are in both)
@Ericson2314
binutils-wrapper: Import separately from cc-wrapper
dbf6d20
@Ericson2314
darwin binutils: Unwrap GNU Binutils before splicing with cctools
b64736a
@Ericson2314
gcc_multi: Fix so binutils wrapper also has glibc_multi
ce359d4
@Ericson2314
rpm: Fix to get raw binutils for headers
b298916
@Ericson2314 Ericson2314 added the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 1, 2017
@Ericson2314
Copy link
Member Author

Everything should be fixed now. https://hydra.mayflower.de/jobset/mayflower/hydra-jobs-cross-rewrite#tabs-evaluations failures are spurious due to outage yesterday.

@Ericson2314
doc: Document Binutils Wrapper
44cc709 
Shrunk the CC Wrapper documentation so as not to be repetative.
@globin
Copy link
Member

globin commented Sep 2, 2017

Please ping me in IRC when you have time later today :)

@Ericson2314 Ericson2314 merged commit 0a944b3 into NixOS:staging Sep 3, 2017
@Ericson2314 Ericson2314 deleted the binutils-wrapper branch September 3, 2017 14:37
@Ericson2314 Ericson2314 added the 8.has: port to stable A PR already has a backport to the stable release. label Sep 3, 2017
edolstra added a commit that referenced this pull request Sep 7, 2017
@edolstra
Revert "Merge pull request #28557 from obsidiansystems/binutils-wrapper"
ec8d41f 
This reverts commit 0a944b3, reversing
changes made to 61733ed.

I dislike these massive stdenv changes with unclear motivation,
especially when they involve gratuitous mass renames like NIX_CC ->
NIX_BINUTILS. The previous such rename (NIX_GCC -> NIX_CC) caused
months of pain, so let's not do that again.
edolstra added a commit that referenced this pull request Sep 7, 2017
@edolstra
Revert "Merge pull request #28557 from obsidiansystems/binutils-wrapper"
460df30 
This reverts commit 0a944b3, reversing
changes made to 61733ed.

I dislike these massive stdenv changes with unclear motivation,
especially when they involve gratuitous mass renames like NIX_CC ->
NIX_BINUTILS. The previous such rename (NIX_GCC -> NIX_CC) caused
months of pain, so let's not do that again.

(cherry picked from commit ec8d41f)
@edolstra
Copy link
Member

edolstra commented Sep 7, 2017

I reverted this in ec8d41f because I really don't want yet another mass rename like NIX_GCC -> NIX_CC, especially when the motivation isn't particular clear to me ("enforce proper layering between binutils and the C compiler" is a bit vague to me).

vcunat added a commit that referenced this pull request Sep 10, 2017
@vcunat
stdenv bootstrap: fix evaluation
c86eb1d 
This is probably a fallout from #28557 merge and revert.
I can't see why exactly this happened, but it seems a safe fix.
vcunat added a commit that referenced this pull request Sep 10, 2017
@vcunat
stdenv bootstrap: fix evaluation
5f15a9f 
This is probably a fallout from #28557 merge and revert.
I can't see why exactly this happened, but it seems a safe fix.

(cherry picked from commit c86eb1d)
vcunat added a commit that referenced this pull request Sep 13, 2017
@vcunat
hpsa service: fallout from #28557 merge and revert
0b131df 
(cherry picked from commit 97ac29c)
vcunat added a commit that referenced this pull request Sep 13, 2017
@vcunat
hpsa service: fallout from #28557 merge and revert
97ac29c
@Ericson2314
Copy link
Member Author

#29396 new version

@samueldr samueldr removed the 9.needs: port to stable A PR needs a backport to the stable release. label Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@copumpkin copumpkin Awaiting requested review from copumpkin

@fpletz fpletz Awaiting requested review from fpletz

@LnL7 LnL7 Awaiting requested review from LnL7

@edolstra edolstra Awaiting requested review from edolstra

@globin globin Awaiting requested review from globin

Assignees
No one assigned
Labels
0.kind: enhancement 1.severity: mass-rebuild 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 6.topic: hygiene 8.has: port to stable A PR already has a backport to the stable release.
Projects
None yet
Milestone
17.09
Development

Successfully merging this pull request may close these issues.
4 participants
@Ericson2314 @globin @edolstra @samueldr