Skip to content

cc-wrapper: Always export environment variables for binutils#27415

Merged
Ericson2314 merged 2 commits intoNixOS:stagingfrom
obsidiansystems:cc-wrapper-prefix-binutils
Jul 16, 2017
Merged

cc-wrapper: Always export environment variables for binutils#27415
Ericson2314 merged 2 commits intoNixOS:stagingfrom
obsidiansystems:cc-wrapper-prefix-binutils

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 16, 2017

Motivation for this change

Before, this only happened when cross compiling. This is less of a hack (and soon the entire preWrap script will go), uniformity is good.

Things done

Built Linux and Darwin stdenvs.


CC @ali-abrar This narrows down the bugs we've seen to the infixSalt-ing, basically.

Before, this only happened when cross compiling.
@mention-bot
Copy link

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @copumpkin and @jwiegley to be potential reviewers.

@Ericson2314 Ericson2314 added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Jul 16, 2017
While this requires newer bash, stdenv's setup.sh now does across the
board anyways. This way is more concise.
@Ericson2314 Ericson2314 force-pushed the cc-wrapper-prefix-binutils branch from c41d2a1 to 3739858 Compare July 16, 2017 00:33
if
PATH=$_PATH type -p "@binPrefix@$CMD" > /dev/null
then
export "${ENV_PREFIX}${CMD^^}=@binPrefix@${CMD}";
Copy link
Member Author

Choose a reason for hiding this comment

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

@Dridus Sorry but I changed it back as new we require newish bash anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I saw, no worries

Copy link
Member

@shlevy shlevy left a comment

Choose a reason for hiding this comment

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

Just glanced through, but LGTM

@Ericson2314 Ericson2314 merged commit 2113631 into NixOS:staging Jul 16, 2017
@Ericson2314 Ericson2314 deleted the cc-wrapper-prefix-binutils branch July 16, 2017 15:21
@Ericson2314
Copy link
Member Author

Well, this one is uncharacteristically small an readable :).

orivej added a commit to orivej/nixpkgs that referenced this pull request Oct 31, 2017
@orivej orivej mentioned this pull request Oct 31, 2017
8 tasks
fpletz pushed a commit that referenced this pull request Nov 1, 2017
Fixes #28282 after #27415

(cherry picked from commit 2ad3933)
@orivej
Copy link
Contributor

orivej commented Dec 19, 2017

IMHO we went in the wrong direction, we should have stopped (and should stop) exporting AR, AS, LD etc. [1] (everything but CC [2]) for cross rather than start exporting them for native, and provide the cross utilities under unprefixed names in the PATH instead. This would have avoided (and will avoid) much debugging and unnecessary workarounds against upstream build scripts, both for native and for cross packaging [3].

[1] This does not apply to BUILD_{var} and {var}_FOR_BUILD.
[2] CC is used consistently upstream and it smooths the difference between gcc and clang.
[3] For example ed will just build, without #32489.

@Ericson2314
Copy link
Member Author

@orivej my original goal was to end up always prefixing all tools with their target platforms (wrapper script only for multi-platform tools). I think the current design makes more sense with that objective.

Yours probably would be epitomized with instead prefixing the binaries with build- and target-, which would also work, and avoid a lot of the of the add-flags.sh and role craziness. But it would mean that buildPackages.buidlPackges.my_compiler (e.g. buildPackages.stdenv.cc) would not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants