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

lib: Clean up how linux and gcc config is specified #107214

Merged
merged 1 commit into from
Jan 22, 2021
Merged

lib: Clean up how linux and gcc config is specified #107214

merged 1 commit into from
Jan 22, 2021

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 19, 2020 •
edited
Loading

Motivation for this change

The platform field is pointless nesting: it's just stuff that happens to be defined together, and that should be an implementation detail.

This instead makes linux-kernel and gcc top level fields in platform configs. They join rustc there [all are optional], which was put there and not in platform in anticipation of a change like this.

linux-kernel.arch in particular also becomes linuxArch, to match the other *Arches.

The next step after is this to combine the specific machines from lib.systems.platforms with lib.systems.examples, keeping just the "multiplatform" ones for defaulting.

Progress on #34274

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 20, 2020
lheckemann
lheckemann reviewed Dec 20, 2020
View reviewed changes
Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

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

Some little corrections, but as you said this is generally still quite WIP so I haven't reviewed everything.

Thoughts:

  • This seems to make sense
  • but what exactly is the purpose of these platforms anyway? Kernel config is quite irrelevant to stdenv, and my impression is that that's the main thing differentiating platforms of the same arch from each other. Meanwhile things like linux's ARCH should probably be implied by system or similar…

lib/systems/platforms.nix Outdated Show resolved Hide resolved
lib/systems/platforms.nix Outdated Show resolved Hide resolved
lib/systems/platforms.nix Outdated Show resolved Hide resolved
lib/systems/platforms.nix Outdated Show resolved Hide resolved
matthewbauer
matthewbauer reviewed Dec 20, 2020
View reviewed changes
lib/systems/default.nix
@@ -79,13 +77,18 @@ rec {
};
isStatic = final.isWasm || final.isRedox;

kernelArch =
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be separate from platform / linux-kernel. This should be a direct mapping from cpu name to linux kernel name. We already have qemuArch and will soon have a "darwinArch" in #105026. This shouldn't be configurable like platform / kernel-arch is.

matthewbauer
matthewbauer reviewed Dec 20, 2020
View reviewed changes
lib/systems/default.nix Outdated
# Just a guess, based on `system`
inherit
(platforms.blank // {
linux-kernel.arch =
Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/NixOS/nixpkgs/pull/107214/files#r546453094

@matthewbauer
Copy link
Member

matthewbauer commented Dec 20, 2020 •
edited
Loading

I don't see much benefit in a rename like this. The Linux kernel options really shouldn't be in crossSystem / hostPlatform at all. They're part of the Linux package and really have nothing to do with the whole package set; different kernels will have different configuration values for instance & there's no reason to special case Linux here. GCC, rustc, and linux-headers make sense though because they are part of the toolchain, and that determines what ABI you are going to use.

@lheckemann lheckemann mentioned this pull request Dec 31, 2020
Merged
1 task
@Ericson2314 Ericson2314 force-pushed the linux-config-cleanup branch 3 times, most recently from 55550c6 to 6d7d62a Compare December 31, 2020 19:40
@Ericson2314 Ericson2314 changed the title WIP: lib: Clean up how linux and gcc config is specified lib: Clean up how linux and gcc config is specified Dec 31, 2020
@Ericson2314
Copy link
Member Author

OK this now builds, thanks for catching all my sloppiness @lheckemann.

@matthewbauer I do agree with lots of that, actually. In particular, compiling a kernel is actually cross compilation, so while I do think kernel config could belong in some hostPlatform (because I don't mind doing all configuration per platform) it doesn't belong in the userland hostPlatform.

That said, I'm not really sure where the headers config ends and kernel config begins? Also do you know where to get the info for a complete-ish linuxArch?

@Ericson2314 Ericson2314 force-pushed the linux-config-cleanup branch 2 times, most recently from b4a4d47 to f381b01 Compare January 11, 2021 04:28
@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 11, 2021 •
edited
Loading

@matthewbauer What do you think now? Even though the linux stuff might be moving again, still think these changes are worth it.

@Ericson2314 Ericson2314 mentioned this pull request Jan 11, 2021
Open
6 tasks
@ofborg ofborg bot added 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 101-500 labels Jan 11, 2021
@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 18, 2021
@ofborg ofborg bot added 8.has: changelog 8.has: documentation and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 22, 2021
@Ericson2314
Copy link
Member Author

I added a release notes for the breaking change. Can we get some final review on this?

matthewbauer
matthewbauer reviewed Jan 22, 2021
View reviewed changes
lib/systems/default.nix
@@ -24,8 +24,6 @@ rec {
# Either of these can be losslessly-extracted from `parsed` iff parsing succeeds.
system = parse.doubleFromSystem final.parsed;
config = parse.tripleFromSystem final.parsed;
# Just a guess, based on `system`
platform = platforms.select final;
Copy link
Member

@matthewbauer matthewbauer Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can we continue to export platform for external usage?

Copy link
Member

Choose a reason for hiding this comment

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

I guess this wouldn't work with the renames though... Maybe it's not worth it.

Copy link
Member Author

@Ericson2314 Ericson2314 Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, true. I was about to do your change below but I didn't think of that.

Copy link
Member Author

@Ericson2314 Ericson2314 Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh post merge I realize the back-compat was already not attempting the linux stuff, oops.

matthewbauer
matthewbauer reviewed Jan 22, 2021
View reviewed changes
lib/systems/default.nix
@@ -24,8 +24,6 @@ rec {
# Either of these can be losslessly-extracted from `parsed` iff parsing succeeds.
system = parse.doubleFromSystem final.parsed;
config = parse.tripleFromSystem final.parsed;
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
config = parse.tripleFromSystem final.parsed;
config = parse.tripleFromSystem final.parsed;
platform = final.linux-kernel // { inherit (final) gcc rustc; };

@Ericson2314
lib: Clean up how linux and gcc config is specified
8929989 
The `platform` field is pointless nesting: it's just stuff that happens
to be defined together, and that should be an implementation detail.

This instead makes `linux-kernel` and `gcc` top level fields in platform
configs. They join `rustc` there [all are optional], which was put there
and not in `platform` in anticipation of a change like this.

`linux-kernel.arch` in particular also becomes `linuxArch`, to match the
other `*Arch`es.

The next step after is this to combine the *specific* machines from
`lib.systems.platforms` with `lib.systems.examples`, keeping just the
"multiplatform" ones for defaulting.
@Ericson2314 Ericson2314 merged commit d95aebb into NixOS:master Jan 22, 2021
@Ericson2314 Ericson2314 deleted the linux-config-cleanup branch January 22, 2021 20:16
@Ericson2314 Ericson2314 mentioned this pull request Jan 22, 2021
Merged
10 tasks
@jonringer
Copy link
Contributor

stdenv rebuild which just got merged to m aster

@jonringer
Copy link
Contributor

jonringer commented Jan 22, 2021 •
edited
Loading

reverted in 0bc275e

Please target staging to avoid the situation where people need to rebuild several hundred packages on PR reviews to master

EDIT: grammar

Ericson2314 added a commit to Ericson2314/nixpkgs that referenced this pull request Jan 23, 2021
@Ericson2314
top-level: Simplify impure and pure fallback
2dde589 
This is now possible, since the `platform` attribute has been removed in
PR NixOS#107214. I've been waiting to do a cleanup like this for a long time!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lheckemann lheckemann lheckemann left review comments

@matthewbauer matthewbauer matthewbauer left review comments

@adisbladis adisbladis Awaiting requested review from adisbladis

@nbp nbp Awaiting requested review from nbp

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 6.topic: stdenv Standard environment 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ericson2314 @matthewbauer @jonringer @lheckemann @SuperSandro2000