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

bc: fix for cross #33478

Merged
merged 1 commit into from Jan 22, 2018
Merged

bc: fix for cross #33478

merged 1 commit into from Jan 22, 2018

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Jan 5, 2018 •
edited

Motivation for this change

Significant rebuild, isolated change from #30882

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@dtzWill dtzWill changed the title bc: Mark appropriate buildInputs as nativeBuildInputs readline, bc: fix for cross Jan 5, 2018
@dtzWill
Copy link
Member Author

dtzWill commented Jan 5, 2018

Fixes bc cross, requires readline fix as well.

Tested with:

nix-build --arg crossSystem '(import ./lib).systems.examples.raspberryPi' -A bc

@globin
Copy link
Member

globin commented Jan 6, 2018

Please base this on staging :)

@dtzWill dtzWill changed the base branch from master to staging January 6, 2018 20:42
@dtzWill
Copy link
Member Author

dtzWill commented Jan 6, 2018

@globin: oops, good call. Done.

@globin
Copy link
Member

globin commented Jan 7, 2018

cc @Ericson2314

dezgeg
dezgeg reviewed Jan 7, 2018
View reviewed changes
pkgs/tools/misc/bc/default.nix Outdated
configureFlags = [ "--with-readline" ];
configureFlags = [
"--with-readline"
"CC_FOR_BUILD=${buildPackages.stdenv.cc.targetPrefix}gcc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be done by stdenv and by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, although actually I think we can omit this for now unless someone has a config where buildPackages' targetPrefix is not empty?

(pushed variant that drops this for now)

Ericson2314
Ericson2314 reviewed Jan 7, 2018
View reviewed changes
Copy link
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.

Hmm ideally we submit this patch somehow, and then pull it from bug tracker or whatever with fetchpatch.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 7, 2018

Readline fix submitted as #33576 , so as to not block on upstreaming the bc patch.

@dtzWill dtzWill changed the title readline, bc: fix for cross bc: fix for cross Jan 9, 2018
@dtzWill
Copy link
Member Author

dtzWill commented Jan 13, 2018

Pushed with small fix for non-cross :).

Ericson2314
Ericson2314 requested changes Jan 14, 2018
View reviewed changes
Copy link
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.

Thanks! Just a few things

pkgs/tools/misc/bc/default.nix Outdated
doCheck = true;
makeFlags = ''HOST_READLINE=${buildPackages.readline.out.outPath}/lib HOST_NCURSES=${buildPackages.ncurses.out.outPath}/lib'';

doCheck = stdenv.hostPlatform == stdenv.buildPlatform;
Copy link
Member

Choose a reason for hiding this comment

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

Can still be true now.

pkgs/tools/misc/bc/default.nix Outdated
patches = [ ./cross-bc.patch ];
nativeBuildInputs = [autoreconfHook flex ed.out texinfo] ++
stdenv.lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform)
[buildPackages.stdenv.cc buildPackages.readline.out buildPackages.ncurses.out];
Copy link
Member

Choose a reason for hiding this comment

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

BuildPackages.stdenv.cc should be a depsBuildBuild, can be unconditional too.

Copy link
Member

Choose a reason for hiding this comment

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

The other two can be made unconditional too, based on the env vars below. Maybe add a comment saying for cross too since people might be confused without the conditional and remove it.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 14, 2018

Eep, that lasted commit isn't looking so good in testing. One sec.

@Ericson2314
Copy link
Member

Weird, thanks for changing though! I'll take a look tomorrow or Monday. My only guess off hand is that turning the makeFlags into env var exports in preConfigure might help.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 18, 2018

rebased onto latest staging, dropped previous attempt to make use of depsBuildBuild.

I think this is finally good to go!

Builds for me natively and cross (tested with aarch64).

@Ericson2314 Ericson2314 added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Jan 18, 2018
@Ericson2314 Ericson2314 self-assigned this Jan 18, 2018
@Ericson2314 Ericson2314 added this to @bgamari's numerous fixes in Cross compilation Jan 18, 2018
@Ericson2314
Copy link
Member

@dtzWill I think we don't need the HOST_ env vars anymore I'll check.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 18, 2018

@Ericson2314 in the patch, you mean? I already dropped them from the expression FWIW.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 18, 2018

@Ericson2314 guess don't need them in either place, pushed commit removing from patch.

@Ericson2314
Copy link
Member

@dtzWill last thing is the native libraries really should be nativeBuildInputs not depsBuildBuild. Weird that this works. I'm going to investigate.

@dtzWill dtzWill requested a review from peti as a code owner January 22, 2018 21:15
@Ericson2314 Ericson2314 removed the request for review from peti January 22, 2018 21:22
@Ericson2314
Copy link
Member

OK, fixed the deps, and made the patch a bit more readable than what the diff algorithm did. Thanks!

@Ericson2314 Ericson2314 merged commit 4e5a4a9 into NixOS:staging Jan 22, 2018
@dtzWill dtzWill deleted the fix/cross-bc branch January 22, 2018 21:59
@dtzWill
Copy link
Member Author

dtzWill commented Jan 22, 2018

Yay! Thanks!

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

@dezgeg dezgeg dezgeg left review comments

@Ericson2314 Ericson2314 Ericson2314 requested changes

Assignees

@Ericson2314 Ericson2314

Labels
6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 10.rebuild-darwin: 1-10 10.rebuild-linux: 501+
Projects
No open projects
Cross compilation
@bgamari's and @dtzWill's num...
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dtzWill @globin @Ericson2314 @dezgeg @GrahamcOfBorg @bgamari