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

Jump to bottom

[WIP] nixpkgs: Begin moving to buildSystem parameter, away from crossSystem #50875

Closed
wants to merge 1 commit into from
Closed

[WIP] nixpkgs: Begin moving to buildSystem parameter, away from crossSystem #50875

wants to merge 1 commit into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Nov 21, 2018 •
edited

In response to #36217, #49510, #49765.

Motivation for this change

The general issue seems to be that localSystem is the wrong abstraction.
It is documented in only one section of the manual, about invoking nixpkgs for cross compilation and apologizing for the current situation. It does not seem to be useful outside the task of invoking nixpkgs. In this issue I (too) am proposing to use the existing build and host terminology, instead of maintaining separate terms local and cross for the singular purpose of invoking nixpkgs.

In a declarative system, we care more about the result than the
process, and the only useful property of the result is hostSystem.
Knowing hostSystem, the only other piece of information we need
in order to derive the previous attributes is buildSystem, which
is extrinsic to the result and can typically be forgotten.

This PR is work in progress. TODO:

  • Test more extensively
  • Decide what the impure defaults should be in top-level/impure.nix
    • Probably best to have buildSystem ? null. Cross-users will have specify buildSystem or we'll have to keep crossSystem around for the purpose of simultaneously setting hostSystem and buildSystem = currentSystem. Any opinions?
  • Implement the impure defaults, in a backwards compatible way
  • Add deprecation message to localSystem and possibly crossSystem
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)
  • Fits CONTRIBUTING.md.

@roberth
nixpkgs: Begin moving to buildSystem parameter, away from crossSystem
48292eb 
The pair of hostSystem and buildSystem is a better abstraction than
localSystem and crossSystem.
The reason is that the latter definitions are optimized for the
build *process* rather than the build *result*. Let me explain:

In a declarative system, we care more about the result than the
process, and the only useful property of the result is *hostSystem*.
Knowing hostSystem, the only other piece of information we need
in order to derive the previous attributes is *buildSystem*, which
is extrinsic to the *result* and can typically be forgotten.
@roberth
Copy link
Member Author

roberth commented Nov 21, 2018

@matthewbauer I'd recommend anyone to avoid 👎 reactions.
However, I do understand that you can have reasons to dislike this change.
My best guess is that I'm asking for feedback too early. Or maybe such a 'philosophical' difference doesn't matter to you? I don't know.

Please write a comment about what needs to improve. It seems that 4 other people would like to see a change like this happen.

@matthewbauer
Copy link
Member

matthewbauer commented Nov 21, 2018 •
edited

Sorry about that! I was going to post a comment but didn’t have time to explain the full reasoning. Basically:

  • buildSystem should be the same thing as localSystem. Renames are fine but this pr seems to treat them differently.
  • hostSystem is such a loaded term that I want to avoid using. “Host” in some contexts is what in Nixpkgs we thing of as “build”. It’s the legacy of autoconf naming.
  • defaulting to null produces too many conditionals IMO. The defaults that i would like would be:
    • localSystem is the system nix autodetects
    • crossSystem == localSystem in native builds

@roberth
Copy link
Member Author

roberth commented Nov 22, 2018

Sorry about that!

No problem!

* buildSystem should be the same thing as localSystem. Renames are fine but this pr seems to treat them differently.

The build and local serve the same purpose, but build doesn't have to be specified by the user if it is the same as hostSystem.

* hostSystem is such a loaded term that I want to avoid using. “Host” in some contexts is what in Nixpkgs we thing of as “build”. It’s the legacy of autoconf naming.

Ok, let's call it runSystem. I'd argue that this one is simple "the" system, but that name seems to be taken.

* defaulting to null produces too many conditionals IMO.

An implementation concern. I'll see if I can rewrite it such that it has fewer conditionals. I do think that the user interface here is more important than the number of conditionals.

The defaults that i would like would be:

  * localSystem is the system nix autodetects
  * crossSystem == localSystem in native builds
  • nothing specified -> build = current, run = current (native build, same behavior)
  • run specified -> build = current, run = args.run (similar to specifying crossSystem)
  • build specified -> build = args.build, run = current (new behavior, useful for doing remote cross builds while using the 'run' device)
  • both specified -> obvious (fully specified, same behavior)

This seems like a very simple "intuitive" defaults scheme to me.

The only issue is that you now have to give two arguments for a remote non-cross build of a foreign architecture; for example a darwin user deploying to linux using a linux build farm.
Some choices:

  • Accept this; require two arguments for a remote build
  • Keep localSystem around for the purpose of a remote building shorthand - mutually exclusive with the other flags after deprecation period
  • Introduce a remoteSystem parameter to serve as a shorthand - mutually exclusive with the other flags. localSystem can be removed after deprecation period
  • Change the runSystem-only invocation to set buildSystem too - require both for cross compilation
  • Change the buildSystem-only invocation to set runSystem too - similar to setting localSystem currently. This complicates the defaults, requiring more documentation etc.
  • Instead of defaulting to currentSystem, we could default to system which then defaults to currentSystem. This means reintroducing a system parameter that may or may not be compatible with a currently obsolete system parameter.

I quite like the latter option, but I don't know enough about the history of cross compilation yet to judge it. It seems quite elegant:
system overrides the default, currentSystem
buildSystem overrides system but only for build
runSystem overrides system but only for run

@Ericson2314
Copy link
Member

mmm agree the autoconf terminology is bad, but if we rename it here we should rename the existing hostPlatform too. I suppose that is more work and more controversy.

More importantly pkgs/top-level/default.nix should only have the new interfaces. pkgs/top-level/impure.nix has existing compat stuff, and this should you there with it.

@mmahut
Copy link
Member

mmahut commented Aug 25, 2019

Are there any updates on this pull request, please?

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@roberth
Copy link
Member Author

roberth commented Jun 1, 2020

TL;DR: Someone please pick this up because this could be much more elegant. Sadly I'm not the right person to do it.

My goal with this PR was to separate the "what" from the "how", where the "what" is most important. I believe that the primary focus of this interface should be the build result, so the platform where the build result can run. Because this is the most essential parameter, I think it should always be set via the same parameter name. Cross builds can then be achieved by changing the platform that builds it.

This is a more top-down design approach than the current bottom-up one.
Nix itself is the result of top-down thinking; taking a step back and designing a system to accomplish a high-level goal. Traditional package managers seem to be the result of a bottom-up approach: what can we build on top of make, tar, etc so we don't have to build everything from source all the time. Anyway, I'm going to cut myself off here to avoid getting too verbose :).

Sadly I'm not the right person to chase this little redesign. I'm not very closely involved with the nixpkgs cross compilation business, so I think that everyone is better off when I leave this and perhaps someone else picks this up. I don't need any credit for this either, because I've only done the easy part.

@roberth roberth closed this Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: work-in-progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@roberth @matthewbauer @Ericson2314 @mmahut @ryantm