Navigation Menu

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.

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

*-wrapper; Switch from infixSalt to suffixSalt #86166

Merged
merged 1 commit into from May 12, 2020
Merged

*-wrapper; Switch from infixSalt to suffixSalt #86166

merged 1 commit into from May 12, 2020

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Apr 28, 2020 •
edited

Motivation for this change

I hate the thing too even though I made it, and rather just get rid of
it. But we can't do that yet. In the meantime, this brings us more
inline with autoconf and will make it slightly easier for me to write a
pkg-config wrapper, which we need.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Ericson2314 Ericson2314 changed the base branch from master to staging April 28, 2020 04:20
@Ericson2314 Ericson2314 force-pushed the suffix-salt branch 4 times, most recently from 6e937f0 to 48263a1 Compare May 1, 2020 14:44
@ofborg ofborg bot added the 6.topic: stdenv Standard environment label May 1, 2020
@Ericson2314 Ericson2314 force-pushed the suffix-salt branch 2 times, most recently from 04ae4ad to 6765930 Compare May 11, 2020 00:22
@Ericson2314
Copy link
Member Author

OK Linux stdenv builds, but on Darwin building Perl package Locale-gettext fails with:

t/bind.t ....... Can't load '/private/tmp/nix-build-perl5.30.2-gettext-1.07.drv-1/Locale-gettext-1.07/blib/arch/auto/Locale/gettext/gettext.bundle' for module Locale::gettext: dlopen(/private/tmp/nix-build-perl5.30.2-gettext-1.07.drv-1/Locale-gettext-1.07/blib/arch/auto/Locale/gettext/gettext.bundle, 2): Symbol not found: _libintl_bind_textdomain_codeset
  Referenced from: /private/tmp/nix-build-perl5.30.2-gettext-1.07.drv-1/Locale-gettext-1.07/blib/arch/auto/Locale/gettext/gettext.bundle
  Expected in: flat namespace
 in /private/tmp/nix-build-perl5.30.2-gettext-1.07.drv-1/Locale-gettext-1.07/blib/arch/auto/Locale/gettext/gettext.bundle at /nix/store/qjr1xvby7aa4mvy4vqr1lyr8hiwy41b6-perl-5.30.2/lib/perl5/5.30.2/darwin-thread-multi-2level/DynaLoader.pm line 197.
 at t/bind.t line 6.

@Ericson2314 Ericson2314 force-pushed the suffix-salt branch 3 times, most recently from 1c7a499 to 5a306aa Compare May 12, 2020 04:16
@Ericson2314
*-wrapper; Switch from infixSalt to suffixSalt
1ac5398 
I hate the thing too even though I made it, and rather just get rid of
it. But we can't do that yet. In the meantime, this brings us more
inline with autoconf and will make it slightly easier for me to write a
pkg-config wrapper, which we need.
Ericson2314
Ericson2314 commented May 12, 2020
View reviewed changes
nixos/doc/manual/release-notes/rl-2009.xml
<listitem>
<para>
The cc- and binutils-wrapper's "infix salt" and <literal>_BUILD_</literal> and <literal>_TARGET_</literal> user infixes have been replaced with with a "suffix salt" and suffixes and <literal>_FOR_BUILD</literal> and <literal>_FOR_TARGET</literal>.
This matches the autotools convention for env vars which standard for these things, making interfacing with other tools easier.
Copy link
Member Author

Choose a reason for hiding this comment

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

In particular I am about to write a pkg-config wrapper for #86077

matthewbauer
matthewbauer reviewed May 12, 2020
View reviewed changes
pkgs/development/tools/build-managers/cmake/default.nix
configureFlags="--parallel=''${NIX_BUILD_CORES:-1} CC=$BUILD_CC CXX=$BUILD_CXX $configureFlags"
''
# CC_FOR_BUILD and CXX_FOR_BUILD are used to bootstrap cmake
+ ''
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be split

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but I like making the comments not change the hashes, so there's no perverse incentive to not keep the docs up to date.

@Ericson2314 Ericson2314 merged commit a0c003e into NixOS:staging May 12, 2020
@Ericson2314 Ericson2314 deleted the suffix-salt branch May 12, 2020 22:38
@Gaelan
Copy link
Contributor

Gaelan commented May 22, 2020 •
edited

@Ericson2314 this PR broke bind, because its build system expects BUILD_CC to be set when cross compiling. Should we fix that by re-adding BUILD_CC to the cc-wrapper setup hook, or just by manually setting BUILD_CC=$CC_FOR_BUILD in the bind derivation?

@Ericson2314
Copy link
Member Author

Ericson2314 commented May 23, 2020 •
edited

@Gaelan Thanks for noticing that! Yes do BUILD_CC=$CC_FOR_BUILD, as BUILD_CC is a less-/non-standard thing.

@Gaelan
Copy link
Contributor

Gaelan commented May 24, 2020

Will do. Also, this seems to make cross-compiled texinfoInteractive fail with a patch rejection; I haven’t figured out why this causes that. I’ll investigate more when I’m in front of my computer.

@erictapen
Copy link
Member

This also broke libgcc, but I have no clue why. @Ericson2314 do you have an idea?

erictapen added a commit to erictapen/nixpkgs that referenced this pull request Jun 15, 2020
@erictapen
bind: set BUILD_CC for cross compilation
6cdbb1f 
This is due to a hint by @Ericson2314 in
NixOS#86166 (comment)
@erictapen erictapen mentioned this pull request Jun 15, 2020
Merged
10 tasks
dguibert added a commit to dguibert/nixpkgs that referenced this pull request Jul 9, 2020
@dguibert
fix pkgsCross.*.texinfoInteractive
a2eb391 
This fixes NixOS#86166 to build cross compilied texinfoInteractive.
BUILD_CC has been renamed to CC_FOR_BUILD so the patch needs to applied
this renaming.

tested on: pkgsCross.armv7l-hf-multiplatform.texinfoInteractive
@dguibert dguibert mentioned this pull request Jul 9, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@infinisil infinisil Awaiting requested review from infinisil infinisil is a code owner

Assignees
No one assigned
Labels
6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Ericson2314 @Gaelan @erictapen @matthewbauer