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

Make cross compilation elegant #26805

Merged
merged 21 commits into from Dec 31, 2017

Conversation

@Ericson2314
Member

Ericson2314 commented Jun 24, 2017

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.

  • Build stdenv
    • Linux
    • Darwin
  • Break up big WIP commit at the end into more readable chunks---perhaps such per-package changes can be merged separately before this PR too. If this is done carefully, the final tree hash will stay the same and we won't mass-rebuild ourselves.
  • Make @globin's doc fixes---see review below.
  • Rewrite documentation to reflect the sorts of dependencies I added, and their new names.
  • optional, will still be an improvement Pass much of cross test suite (besides including MinGW) http://hydra.nixos.org/jobset/nixpkgs/ericson2314-cross-trunk
  • Put (propagated)buildInputs back on the PATH when not cross-compiling, to lessen potential breakage.---but I want to make sure at least the stdenv builds before I do this.
  • Don't prune native of native, to lessen potential breakage. Breakage dealt with despite this
  • Prevent regressions
  • Fix Qt derivations based on what @ttuegel said.
  • Clean up propagation description in documentation
  • Track down any more unwanted __ (some in docs, it seems)
  • Add release notes for NixOS

CC @Dridus @matthewbauer @DavidEGrayson @bgamari

@bjornfor

This comment has been minimized.

Show comment
Hide comment
@bjornfor

bjornfor Jun 24, 2017

Contributor

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".

Contributor

bjornfor commented Jun 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bjornfor

bjornfor Jun 24, 2017

Contributor

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 :-)

Contributor

bjornfor commented Jun 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jun 25, 2017

Member

OK renamed patches and overhauled documentation.

Member

Ericson2314 commented Jun 25, 2017

OK renamed patches and overhauled documentation.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jun 26, 2017

Member

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.

Member

Ericson2314 commented Jun 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@copumpkin

copumpkin Jun 26, 2017

Member

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 😄

Member

copumpkin commented Jun 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@copumpkin

copumpkin Jun 26, 2017

Member

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.

Member

copumpkin commented Jun 26, 2017

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.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jun 26, 2017

Member

@copumpkin It's still a fair point :). Part of the problem is I'm often not sure how to split things up better, but definitely another is me writing some some big rewrite I've already planned in my head. As big as these PRs are, they do build on each other---but that just means to really understand them it would be easiest for someone who had reviewed every single one from the beginning!

I just tried to separate things out a little bit: I split out the docs, and the (imo boring) splicing change, so now it's the 4th commit that causes the infinite recursion. I could also make the 1st commit a separate PR from the rest---that one was indeed easier to review and so I got a bunch of great feedback.

Member

Ericson2314 commented Jun 26, 2017

@copumpkin It's still a fair point :). Part of the problem is I'm often not sure how to split things up better, but definitely another is me writing some some big rewrite I've already planned in my head. As big as these PRs are, they do build on each other---but that just means to really understand them it would be easiest for someone who had reviewed every single one from the beginning!

I just tried to separate things out a little bit: I split out the docs, and the (imo boring) splicing change, so now it's the 4th commit that causes the infinite recursion. I could also make the 1st commit a separate PR from the rest---that one was indeed easier to review and so I got a bunch of great feedback.

Ericson2314 added some commits Sep 4, 2017

gcc: Fix deps, for cross and consistency
Mainly making sure we have tools to build target libs
gcc, binutils: Get rid of 32-bit ARM configure flag exception
Now that we do `--enable-targes=all`, there is no risk of missing the
needed emulation.

This reverts commit ebc9b16.
This reverts commit 88efc22.
stdenv-setup: Ease the transition with native builds
 - 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.
kdoctools: Perl is a propagated *run*-time dep
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

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Dec 31, 2017

Member

@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. :)

Member

Ericson2314 commented Dec 31, 2017

@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. :)

Ericson2314 added some commits Jun 2, 2017

doc: Add "Specifying Dependencies" section to the stdenv chapter
This accounts for all the new dependencies and propagation logic changes
I'm about to add.

Fixes #1915---with this change I think the distinction is finally clear
enough.
doc: Add cross cookbook
An excellent suggestion from @bgamari

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

8 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env --file . --query --available --json
Details

@Ericson2314 Ericson2314 deleted the obsidiansystems:cross-elegant branch Dec 31, 2017

@Ericson2314 Ericson2314 referenced this pull request Dec 31, 2017

Closed

wiki: CrossCompiling #13203

@CMCDragonkai

This comment has been minimized.

Show comment
Hide comment
@CMCDragonkai

CMCDragonkai Dec 31, 2017

Contributor

Wow this is merged now! Congrats!

Contributor

CMCDragonkai commented Dec 31, 2017

Wow this is merged now! Congrats!

@bjornfor

This comment has been minimized.

Show comment
Hide comment
@bjornfor

bjornfor Jan 3, 2018

Contributor

I think the doCheck example is wrong. It should be doCheck = stdenv.hostPlatform == stdenv.buildPlatform, right?

Contributor

bjornfor commented on doc/cross-compilation.xml in 8a434b6 Jan 3, 2018

I think the doCheck example is wrong. It should be doCheck = stdenv.hostPlatform == stdenv.buildPlatform, right?

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jan 3, 2018

Member

Ooops, hahahaha

Member

Ericson2314 replied Jan 3, 2018

Ooops, hahahaha

@periklis

This comment has been minimized.

Show comment
Hide comment
@periklis

periklis Jan 4, 2018

Contributor

@Ericson2314 & @bgamari Nice to see that the patterns made it to docs as cookbook. Thx.

Contributor

periklis commented on 8a434b6 Jan 4, 2018

@Ericson2314 & @bgamari Nice to see that the patterns made it to docs as cookbook. Thx.

@shlevy

This comment has been minimized.

Show comment
Hide comment
@shlevy

shlevy Jan 4, 2018

Member

@Ericson2314 This breaks emacsWithPackages, which used the previous findInputs interface.

Member

shlevy commented on 7f3ca3e Jan 4, 2018

@Ericson2314 This breaks emacsWithPackages, which used the previous findInputs interface.

@srhb srhb referenced this pull request Jan 6, 2018

Merged

gettext: Don't use envHooks anymore #33524

1 of 8 tasks complete

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

@timbertson timbertson referenced this pull request Feb 18, 2018

Closed

Various fixes #12

@matthewbauer

This comment has been minimized.

Show comment
Hide comment
@matthewbauer

matthewbauer Mar 25, 2018

Member

This shouldn't be twice, right?

Member

matthewbauer commented on pkgs/build-support/emacs/setup-hook.sh in 046f091 Mar 25, 2018

This shouldn't be twice, right?

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Mar 26, 2018

Member

haha yeah that's a bug. Maybe I meant to do build and host cause no idea which one was correct and didn't want to miss anything, but that is before I did the blanket native-compat hack so it shouldn't be needed.

Member

Ericson2314 replied Mar 26, 2018

haha yeah that's a bug. Maybe I meant to do build and host cause no idea which one was correct and didn't want to miss anything, but that is before I did the blanket native-compat hack so it shouldn't be needed.

@matthewbauer

This comment has been minimized.

Show comment
Hide comment
@matthewbauer
Member

matthewbauer commented on pkgs/stdenv/generic/make-derivation.nix in 7f3ca3e Jul 17, 2018

/cc @Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jul 28, 2018

Member

Thanks. Fixed on staging

Member

Ericson2314 replied Jul 28, 2018

Thanks. Fixed on staging

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