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

top-level: Create pkgs{Build,Host,Target}{Build,Host,Target} #57611

Merged
merged 6 commits into from Mar 26, 2019

Conversation

Ericson2314
Copy link
Member

Motivation for this change
top-level: Create pkgs{Build,Host,Target}{Build,Host,Target}

This is needed to avoid confusing and repeated boilerplate for
fooForTarget. The vast majority of use-cases can still use
buildPackages or targetPackages`, which are now defined in terms of
these.

ghc, go, guile: Use new pkgs*

pkgsBuildTarget allows us to avoid repeated and confusing conditions.
The others merely provide clarity for one the foreign package set's
target platform matters.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.


# Older names for package sets. Use these when only the host platform of the
# package set matter (i.e. use `buildPackages` where any of `pkgsBuild*`
# would do, and `targetPackages` when any of `pkgsTarget*` would do.)
Copy link
Member

Choose a reason for hiding this comment

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

There’s only one pkgsTarget* right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I should make that clear I'm saying would there be more than one it wouldn't matter.

# pkgsTargetTarget ...;
# }
#
# These are references to adjacent bootstrapping stages. The more familiar
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put this in Nixpkgs documentation.

else assert targetPlatform == hostPlatform; # build != host == target
[ stdenv.cc ] ++ stdenv.lib.optional useLLVM buildLlvmPackages.llvm;
toolsForTarget = [
pkgsBuildTarget.targetPackages.stdenv.cc
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need targetPackages here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because stdenv.cc comes from the previous stage. targetPackages.stdenvsort of cancels out so I get something likepkgsBuildTarget.gccorpkgsBuildTarget.llvm` in the end.

@@ -22,7 +22,7 @@

depsBuildBuild = [ buildPackages.stdenv.cc ]
Copy link
Member

Choose a reason for hiding this comment

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

Should pkgsBuildBuild be used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would have to be pkgsBuildBuild.targetPackages.stdenv.cc, so ehh hardly worth it for now.

# platform. We only care about their host/target platforms, not their build
# platform, because the the former two alone affect the interface of the
# final package; the build platform is just an implementation detail that
# should not leak.
Copy link
Member

Choose a reason for hiding this comment

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

Please put also this into the documentation.

@Ericson2314 Ericson2314 requested a review from shlevy March 22, 2019 17:59
@Ericson2314
Copy link
Member Author

CC @shlevy here is that change the reduces the importance of the sliding window.

if args.stdenv.hostPlatform == args.stdenv.targetPlatform
then thisStage
else assert args.stdenv.buildPlatform == args.stdenv.hostPlatform; pkgsBuildHost;
pkgsTargetTarget = nextStage;
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the proof while stages are "braided" not "chained" (more edges between them than buildPackages targetPackages), there are no more stages than before (same nodes).

Copy link
Member Author

Choose a reason for hiding this comment

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

But if we ever had any stages that were canadian cross (build != host != target) then we would need more stages, as no {linear, sliding window}-generated stage would fit pkgsBuildTarget or pkgsHostHost. This relates to those two being conditionally hacked together above from the linear ones, while the rest are statically chosen.

@Ericson2314 Ericson2314 force-pushed the stage-braid-not-chain branch 2 times, most recently from b494854 to 5aed659 Compare March 24, 2019 20:35
Ericson2314 and others added 5 commits March 24, 2019 22:11
$(shell ...) looks a little sketch like it will be run no matter what.
And there are problems building the manual on darwin so hopefully this
fixes them.
This is needed to avoid confusing and repeated boilerplate for
`fooForTarget`.  The vast majority of use-cases can still use
`buildPackages or `targetPackages`, which are now defined in terms of
these.
`pkgsBuildTarget` allows us to avoid repeated and confusing conditions.
The others merely provide clarity for one the foreign package set's
target platform matters.
There was a bunch of stuff in the cross section that haddn't had any
attention in a while. I might need to slim it down later, but this is
good for now.
that packages should be written with cross-compilation in mind, and nixpkgs
should evaluate in a similar way (by minimizing cross-compilation-specific
special cases) whether or not one is cross-compiling.
run-time environments! Significant, because the benefits apply even when one
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run-time environments! Significant, because the benefits apply even when one
run-time environments!

should evaluate in a similar way (by minimizing cross-compilation-specific
special cases) whether or not one is cross-compiling.
run-time environments! Significant, because the benefits apply even when one
is developing and deploying on the same machine. Nixpkgs is increasingly
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
is developing and deploying on the same machine. Nixpkgs is increasingly
Nixpkgs is increasingly

review changes from pull request number 12345:
<screen>nix-shell -p nix-review --run "nix-review pr 12345"</screen>
review changes from pull request number 12345:
<screen>nix-shell -p nix-review --run "nix-review pr 12345"</screen>
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is the autoformatter.

doc/stdenv.xml Outdated Show resolved Hide resolved
likely to be caught by CI and other users.
</para>
<para>
Thirdly, it is because everything target-mentioning only exists to
Copy link
Member

Choose a reason for hiding this comment

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

I disagree. A compiler will never be able to build everything, so the you will always need to keep track of target. Things like LLVM get close to compiling everything, but we might prefer to only build the part of LLVM we need.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I think part of the answer is targetPlatform is less bad than targetPackages. I'll merge this now to unblock other stuff, but get back to fixing that wording later.

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

grammar / edits / etc.

Thanks @matthewbauer!

Co-Authored-By: Ericson2314 <git@JohnEricson.me>
@Ericson2314 Ericson2314 merged commit aa0cf64 into NixOS:master Mar 26, 2019
@Ericson2314 Ericson2314 deleted the stage-braid-not-chain branch March 26, 2019 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants