Make cross compilation elegant by Ericson2314 · Pull Request #26805 · NixOS/nixpkgs

Ericson2314 · 2017-06-24T00:28:30Z

Motivation for this change

#26881 has the easy cleanups of individual derivations, but there is some more complex cleanups to core infrastructure that should also be made. This PR is for them.

Because these changes are both complex, and cause huge mass rebuilds, it will take longer to get right. I might or might not might land #26799 or #25047 in the meantime, for example. Opening now because such changes warrant much discussion.

When #26881 is merged, I'll repoint those hydra jobs at this.

Things done

@periklis I opened https://github.com/NixOS/nixpkgs/projects/8 as a meta-issue; also see these new TODOs below.

CC @Dridus @matthewbauer @DavidEGrayson @bgamari

bjornfor · 2017-06-24T09:40:32Z

pkgs/development/libraries/zlib/default.nix

Shouldn't this be PREFIX=${hostPlatform.config}-? (Or at least not stdenv.cross ...?)

Good catch!

bjornfor · 2017-06-24T09:45:03Z

pkgs/tools/misc/coreutils/default.nix

Inverted logic? Isn't it so that when you're building for host != build, then you'd like to use "install" from the buildPackages set?

bjornfor · 2017-06-24T09:46:25Z

pkgs/tools/misc/coreutils/default.nix

Same inverted logic?

bjornfor · 2017-06-24T10:05:16Z

pkgs/stdenv/generic/setup.sh

Should one of these "m"s be "n"?

hehe, too much copy paste

bjornfor · 2017-06-24T10:16:46Z

The commit message in "stdenv: Fix handling of dependencies and hooks" says

"... crops up more that one might think it old commonly used C libraries". I think you mean "in", not "it".
"... pick up any libraries for it's target platform". it's -> its.

I think it'd be nice if the documentation in the commit message was added to "pkgs/stdenv/generic/setup.sh".

bjornfor · 2017-06-24T10:18:57Z

I have nothing to say about the code other than the nitpicks above. Looks good to me!

I'm always happy to see you work on cross-compilation :-)

Ericson2314 · 2017-06-24T14:46:58Z

pkgs/tools/system/ddrescue/default.nix

This should have been configurePlatforms, but that wasn't shows that isn't needed after all!

Mic92 · 2017-06-25T08:11:11Z

pkgs/tools/archivers/unzip/default.nix

this does not look right.

Actually it is. I changed makefile to only define the c compiler as a fallback: CC ?= cc. What I can do is give the patch a better name :)

What I meant the trailing semicolon might be unintended, isn't it?

Ericson2314 · 2017-06-25T22:31:39Z

OK renamed patches and overhauled documentation.

Ericson2314 · 2017-06-26T06:07:59Z

Cleaned up cc-wrapper, and made misc packages use preNativeBuildInputs too.

The stdenv commit unfortunately causes some infinite reversion. No idea why; if anyone could figure out that would be much appreciated.

copumpkin · 2017-06-26T13:24:40Z

It really makes it pretty hard to review or follow things like this when every change on this topic is a batch of commits with a total of several hundred lines of changes, often later getting closed with "I redid all this work in PR 12345". Like you saying, "hey the third commit of six has an obscure bug I haven't been able to figure out; please help!" might be a hint that you're lumping too much together. Especially when that problematic commit then has a commit message starting with "3 big changes: ". It's just daunting to me, and I doubt I'm alone.

Don't get me wrong: I appreciate all the work you're putting in here, and appreciate descriptive commit messages, but I think this tweet sums up my position on code reviews. Too small and they tend to be nitpicky, but too big and there's just too much going on for anyone to get enough context to provide a meaningful review without investing an hour or more into it, which is more than most people are willing to put in. If your goal as a code author is to get meaningful feedback from other people with relevant expertise, I'd urge you to not only optimize your code for correctness (which you're probably fine with) but optimize your coding practice to enable the rest of us to see that it's correct. That'll mean smaller, more focused commits, more focused PRs, even if that sometimes means not writing a feature the way it comes naturally to you, in order to make it more incremental and consumable by the rest of us.

Anyway, that's just my two cents. Please don't take it as anything more than what I'm saying. I really want your stuff to land and generally love and appreciate all the cross-compilation revamp and cleanup work you've been doing, but I fairly consistently can make very little sense of your PRs. That might just be me, of course 😄

copumpkin · 2017-06-26T13:26:55Z

Okay, I take some of that back, sorry. After looking more carefully at the stdenv commit in question, I now see that most of the changes are documentation changes, which I really appreciate, and the remainder is fairly small logic. I just knee-jerked too hard at the "3 big changes" in the commit message. Anyway, given that, I don't know if there's much remaining truth to my original point but I'll leave it up and see what you think.

This commits needs a MAJOR audit as I oftentimes just guessed which of `$hostOffset`, `$targetOffset`, or a fixed offset should be used.

libgcc.a and similar

- All deps go on the PATH - CC and Bintools wrappers with their host != depender's host still get their setup hooks run. - Environment hooks get applied to all packages This isn't so elegent, but eases the transition on a very significant PR.

It was improperly classified a build-time dep to get around the incorrect propagation logic that was in place before this PR. Additionally fix some `kdoctools` usage were it is incorrectly used a run-time dep.

Ericson2314 · 2017-12-31T03:29:07Z

@bgamari Thanks for your very thoughtful and thorough review!

I fixed everything I could (including start cookbook with your examples) that wouldn't cause a mass rebuild (e.g. more comments in pkgs/generic/setup.sh). Those I'll make after and merge as we work through #30882. That way the last few stalled-looking jobs finish OK (We can already see how they turned out before, but just to double check.)

I'll be around a lil bit tomorrow to help put out any fires (e.g. some rustc on Darwin) that may arise.

Code-churn wise, this cross saga is all downhill from hill. :)

This accounts for all the new dependencies and propagation logic changes I'm about to add. Fixes NixOS#1915---with this change I think the distinction is finally clear enough.

@bgamari

An excellent suggestion from @bgamari

CMCDragonkai · 2017-12-31T05:21:27Z

Wow this is merged now! Congrats!

andersk · 2021-08-03T23:32:42Z

pkgs/stdenv/generic/setup.sh

+
+        # Ensure we're in bounds relative to the package currently
+        # being built.
+        [[ "${allPlatOffsets[*]}" = *"$hostOffsetNext"*  ]] || continue


This naïve substring match considers 1 to be a substring of -1. Did you mean to write this?

- [[ "${allPlatOffsets[*]}" = *"$hostOffsetNext"* ]] || continue + [[ " ${allPlatOffsets[*]} " = *" $hostOffsetNext "* ]] || continue

Ericson2314 added 1.severity: mass-rebuild 6.topic: cross-compilation Building packages on a different platform than they will be used on 9.needs: community feedback This needs feedback from more community members. labels Jun 24, 2017

Ericson2314 self-assigned this Jun 24, 2017

Ericson2314 requested review from LnL7, bjornfor, dezgeg and vcunat June 24, 2017 00:28

Ericson2314 mentioned this pull request Jun 24, 2017

haskell infra: Fix cross compilation to work with new system #26799

Merged

9 tasks

Ericson2314 force-pushed the cross-elegant branch from 2fa41a1 to 8de191a Compare June 24, 2017 00:35

bjornfor reviewed Jun 24, 2017

View reviewed changes

pkgs/tools/misc/coreutils/default.nix Outdated

Copy link

Contributor

bjornfor Jun 24, 2017

Choose a reason for hiding this comment

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

Same inverted logic?

bjornfor reviewed Jun 24, 2017

View reviewed changes

Ericson2314 commented Jun 24, 2017

View reviewed changes

Ericson2314 force-pushed the cross-elegant branch 2 times, most recently from eba946a to e388be3 Compare June 24, 2017 15:15

Ericson2314 mentioned this pull request Jun 25, 2017

pkgs/stdenv/generic/setup.sh taints PATH with unexecutable binaries during cross build #21191

Closed

Mic92 reviewed Jun 25, 2017

View reviewed changes

Ericson2314 force-pushed the cross-elegant branch from e388be3 to f6a1d58 Compare June 25, 2017 22:31

Ericson2314 force-pushed the cross-elegant branch 3 times, most recently from ba655a4 to b80754c Compare June 26, 2017 06:03

Ericson2314 added 5 commits December 30, 2017 22:04

treewide: Don't use envHook anymore

046f091

This commits needs a MAJOR audit as I oftentimes just guessed which of `$hostOffset`, `$targetOffset`, or a fixed offset should be used.

treewide: Don't use nativePkgs or crossPkgs anymore

e0f1739

linux bootstrap tools cross: Nuke more refs

f083248

libgcc.a and similar

kdoctools: Perl is a propagated *run*-time dep

e9a369b

It was improperly classified a build-time dep to get around the incorrect propagation logic that was in place before this PR. Additionally fix some `kdoctools` usage were it is incorrectly used a run-time dep.

Ericson2314 force-pushed the cross-elegant branch from 280af3a to 626136a Compare December 31, 2017 03:08

doc: Add "Specifying Dependencies" section to the stdenv chapter

7ffc4e1

This accounts for all the new dependencies and propagation logic changes I'm about to add. Fixes NixOS#1915---with this change I think the distinction is finally clear enough.

Ericson2314 force-pushed the cross-elegant branch from 626136a to bfe3b82 Compare December 31, 2017 03:38

Ericson2314 and others added 4 commits December 30, 2017 22:41

doc: Document dontStripHost and dontStripTarget

b55999b

doc: More information in the setup hook section of the stdenv chapter

edfe42f

doc: Add cross cookbook

8a434b6

An excellent suggestion from @bgamari

doc: Breaking change release not for dependency propagation logic

a98e686

Ericson2314 force-pushed the cross-elegant branch from bfe3b82 to a98e686 Compare December 31, 2017 03:42

Ericson2314 merged commit 4d2b763 into NixOS:staging Dec 31, 2017

Ericson2314 deleted the cross-elegant branch December 31, 2017 03:59

Ericson2314 mentioned this pull request Dec 31, 2017

wiki: CrossCompiling #13203

Closed

This was referenced Dec 31, 2017

On Darwin, Haskell packages that use hsc2hs cannot be built with GHCJS due to missing libiconv #25665

Closed

Unable to cross compile pkgs.gcc #25212

Closed

boost: Do not force numpy support from 1.65 #33359

Merged

hedning mentioned this pull request Jan 5, 2018

nautilus-sendto 3.8.6 fails to build on NixOS #33458

Closed

srhb mentioned this pull request Jan 6, 2018

gettext: Don't use envHooks anymore #33524

Merged

8 tasks

ryantrinkle mentioned this pull request Jan 24, 2018

SLURP: a Single Liberal Unified Registry of Haskell Packages haskell/ecosystem-proposals#4

Open

timbertson added a commit to timbertson/opam2nix-packages that referenced this pull request Feb 18, 2018

fix broken envHooks after NixOS/nixpkgs#26805

07979a6

timbertson mentioned this pull request Feb 18, 2018

Various fixes timbertson/opam2nix-packages#12

Closed

andersk reviewed Aug 3, 2021

View reviewed changes

ShamrockLee mentioned this pull request Oct 13, 2022

Ambiguous choice of setup hooks document location #195759

Closed

3 tasks

pbsds mentioned this pull request Dec 8, 2023

Unwanted boilerplate: finalAttrs.finalPackage.doCheck #272978

Open

ConnorBaker mentioned this pull request Mar 10, 2025

Stdenv setup: Process dependencies before dependents #31414

Closed

8 tasks

Uh oh!

Conversation

Ericson2314 commented Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjornfor commented Jun 24, 2017

Uh oh!

bjornfor commented Jun 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mic92 Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Jun 25, 2017

Uh oh!

Ericson2314 commented Jun 26, 2017

Uh oh!

copumpkin commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copumpkin commented Jun 26, 2017

Uh oh!

Ericson2314 commented Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CMCDragonkai commented Dec 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

Ericson2314 commented Jun 24, 2017 •

edited

Loading

Mic92 Jun 26, 2017 •

edited

Loading

copumpkin commented Jun 26, 2017 •

edited

Loading

Ericson2314 commented Dec 31, 2017 •

edited

Loading