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

treewide: Properly depend on autoreconfHook and pkg-config #29039

Merged
merged 20 commits into from Sep 28, 2017
Merged

treewide: Properly depend on autoreconfHook and pkg-config #29039

merged 20 commits into from Sep 28, 2017

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 5, 2017 •
edited

Motivation for this change

pkg-config and autoreconfHook should always be a nativeBuildInputs rather than buildInputs. This big PR makes it so in most cases (when grep doesn't fail me). Additionally, we can get rid of the cross hacks to attempt to categorize them, now that it is fixed properly. Post-#26805 this may actually be necessary to keep the build from failing, too.

Things done

https://hydra.mayflower.de/jobset/mayflower/hydra-jobs-cross-rewrite built some of this on linux.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built stdenv on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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.

@Ericson2314 Ericson2314 added the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 5, 2017
@Ericson2314 Ericson2314 changed the title Deps reorg misc pkgs: Properly depend on pkg-config Sep 5, 2017
@Ericson2314 Ericson2314 added this to the 17.09 milestone Sep 5, 2017
@Ericson2314 Ericson2314 added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Sep 5, 2017
@Ericson2314 Ericson2314 mentioned this pull request Sep 5, 2017
Merged
8 tasks
@Ericson2314 Ericson2314 added this to Needed by the big PR---nice to move pick off pieces of it and move here, rebasing the big PR on top in Cross compilation Sep 5, 2017
@Ericson2314 Ericson2314 mentioned this pull request Sep 5, 2017
Merged
13 tasks
@dezgeg dezgeg requested a review from edolstra September 6, 2017 01:13
@dezgeg
Copy link
Contributor

dezgeg commented Sep 6, 2017

While a good idea in general, definitely not 17.09 material.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 6, 2017 •
edited

I disagree for the reasons I said above. To elaborate, large refactors that churn a lot of code but are highly unlikely to break anything are best done before releases, so later cherry-picks from master to stable do work. It would be ideal to do this before the fork-off for ease and simpler history, but I don't mind the extra work recreating this on master, and there's many other things already making the history complex.

Additionally, it is likely this will be needed for #26805. I have talked extensively with our release managers, @globin and @fpletz on landing that by 17.09---while there is a still a fair amount of code motion, build-time breakage will be easy to detect and run-time breakage is highly unlikely as no versions are changing.

@Ericson2314 Ericson2314 force-pushed the deps-reorg branch 2 times, most recently from a7aec95 to 152056f Compare September 6, 2017 15:30
@Ericson2314
Copy link
Member Author

I added a commit simplifying and making more forward-compatable autoreconfHook, mainly so
https://hydra.mayflower.de/jobset/mayflower/hydra-jobs-cross-rewrite#tabs-evaluations would pick it up. I'll probably end up making a separate PR for it however, for organization's sake.

@Ericson2314
Copy link
Member Author

@fpletz @globin I think https://hydra.mayflower.de/jobset/mayflower/hydra-jobs-cross-rewrite#tabs-evaluations might have stalled? The queue hasn't budged for a few hours. [Also, as I mentioned on IRC, I think the linux failures are spurious.]

FRidh
FRidh reviewed Sep 9, 2017
View reviewed changes
pkgs/development/python-modules/Cython/default.nix
nativeBuildInputs = [
pkgconfig
# For testing
numpy ncurses
Copy link
Member

Choose a reason for hiding this comment

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

Do these actually need to be native?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not. Did this come from a minimal edit? I'll check

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it did. @FRidh I agree it looks weird, but probably better to fix separately. Because this one is so big, I want to keep it boring.

@Ericson2314 Ericson2314 changed the title misc pkgs: Properly depend on pkg-config treewide: Properly depend on pkg-config Sep 14, 2017
@Ericson2314 Ericson2314 removed this from the 17.09 milestone Sep 14, 2017
@Ericson2314 Ericson2314 removed the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 14, 2017
@Ericson2314 Ericson2314 removed this from Needed by the big PR---nice to move pick off pieces of it and move here, rebasing the big PR on top in Cross compilation Sep 14, 2017
@Ericson2314 Ericson2314 changed the base branch from release-17.09 to staging September 14, 2017 16:09
@Ericson2314
Copy link
Member Author

No longer slated for 17.09.

@peti
Copy link
Member

peti commented Sep 27, 2017

What is the state of this PR?

@Ericson2314
Copy link
Member Author

It works; I'm currently using it. But, it bitrots lightening quick. If someone wants to give it a final review, tell me when they will and I'll fix it just before then. If that goes well, I should probably merge.

@NeQuissimus
Copy link
Member

LGTM, just needs a rebase :)

@edolstra
Copy link
Member

Looks good to me too.

@Ericson2314
Copy link
Member Author

Thanks!

@Ericson2314 Ericson2314 merged commit 9a0f593 into NixOS:staging Sep 28, 2017
@Ericson2314 Ericson2314 deleted the deps-reorg branch September 28, 2017 16:36
@Ericson2314 Ericson2314 mentioned this pull request Sep 29, 2017
Merged
vcunat added a commit that referenced this pull request Oct 7, 2017
@vcunat
xorg-server: fixup build after #29039
35b4c8b 
The expression uncarefully overrode whole nativeBuildInputs,
so after the pkgconfig move it got removed and the build failed.
1.19.3 doesn't need these lines anyway...
vcunat added a commit that referenced this pull request Oct 7, 2017
@vcunat
texlive.bin.core-big: fixup build after #29039
656a008
vcunat added a commit that referenced this pull request Oct 7, 2017
@vcunat
dmtx: fixup build after #29039
b174f42
vcunat added a commit that referenced this pull request Oct 7, 2017
@vcunat
gnome3.vte-ng: fixup build after #29039
55ed948
vcunat added a commit that referenced this pull request Oct 8, 2017
@vcunat
xf86-video-intel: fixup after #29039
35f8948 
This was one package in pkgs/servers/x11/xorg/default.nix
that was *not* generated, and that was missed in the PR
and overwritten (understandably).
Now the change is properly in overrides.nix to prevent that...
@vcunat
Copy link
Member

vcunat commented Oct 8, 2017

As for "unlikely to break anything", I've spent a few hours fixing regressions from this. I think most changes with large "code churn" have a breaking potential.

@Ericson2314
Copy link
Member Author

Thanks for making those fixes, @vcunat. I'm sorry I wasn't around this weekend to help.

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

@FRidh FRidh FRidh left review comments

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

@fpletz fpletz Awaiting requested review from fpletz

@edolstra edolstra Awaiting requested review from edolstra

@copumpkin copumpkin Awaiting requested review from copumpkin

@LnL7 LnL7 Awaiting requested review from LnL7

@peti peti Awaiting requested review from peti

Assignees

@edolstra edolstra

Labels
6.topic: cross-compilation Building packages on a different sort platform than than they will be run on
Projects
No open projects
Cross compilation
Needed by the big PR---nice to move p...
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Ericson2314 @dezgeg @DavidEGrayson @peti @NeQuissimus @edolstra @vcunat @FRidh