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

Add CPU model support to target platform #313539

Closed
wants to merge 8 commits into from

Conversation

RossComputerGuy
Copy link
Member

@RossComputerGuy RossComputerGuy commented May 22, 2024

Description of changes

Adds a cpuModel attribute to targetPlatform and defaults the CPU model to "generic". Might need a lot of work but I think this is a great start. We might want to check the CPU model based on the CPU arch coming in and have a fallback of only "generic" when we cannot match a recognized CPU arch.

Fixes #313530

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: lib The Nixpkgs function library 6.topic: zig labels May 22, 2024
@RossComputerGuy RossComputerGuy force-pushed the feat/cpumodel branch 2 times, most recently from d22bb42 to aa8c6a6 Compare May 22, 2024 02:41
@Ericson2314
Copy link
Member

This seems close to https://github.com/NixOS/nixpkgs/blob/master/lib/systems/architectures.nix ? My thought at the time with that is that I don't really know what the difference between an instruction set architecture and a "sub instruction set architecture", except for rather arbitrary conventions about what goes in the target triple or not, which I view as sort of historical accident we shouldn't take too seriously.

@RossComputerGuy
Copy link
Member Author

RossComputerGuy commented May 22, 2024

This seems close to https://github.com/NixOS/nixpkgs/blob/master/lib/systems/architectures.nix ? My thought at the time with that is that I don't really know what the difference between an instruction set architecture and a "sub instruction set architecture", except for rather arbitrary conventions about what goes in the target triple or not, which I view as sort of historical accident we shouldn't take too seriously.

Yeah, though this mechanism is simpler. I'm not sure how that method works in Nixpkgs but this way you just add your CPU model to the end of config. It also allows for easy checking of common CPU models so marking certain packages as broken can be simpler. Adding this into the config stuff also allows for easy cross compiling via systems.examples. Also, this is compatible with LLVM, Zig, and GCC. The way things in Nixpkgs are done seems more GNU based.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label May 22, 2024
@github-actions github-actions bot removed the 6.topic: stdenv Standard environment label May 22, 2024
@RossComputerGuy RossComputerGuy force-pushed the feat/cpumodel branch 5 times, most recently from befa041 to 34cad61 Compare May 22, 2024 05:34
@RossComputerGuy
Copy link
Member Author

Idk what is up with by-name failing. I cannot reproduce that error with the information given.

@RossComputerGuy RossComputerGuy force-pushed the feat/cpumodel branch 3 times, most recently from 45eb99b to f04dc9f Compare May 22, 2024 06:00
@RossComputerGuy
Copy link
Member Author

Just cleaned up the commits, fixed some of the issues found with CI. Made it so this doesn't rebuild everything if the CPU model is generic. I think this is at a state that I am happy.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 22, 2024
@RossComputerGuy
Copy link
Member Author

@ofborg build pkgsCross.asahi-m1.bash

@alyssais
Copy link
Member

This seems close to https://github.com/NixOS/nixpkgs/blob/master/lib/systems/architectures.nix ? My thought at the time with that is that I don't really know what the difference between an instruction set architecture and a "sub instruction set architecture", except for rather arbitrary conventions about what goes in the target triple or not, which I view as sort of historical accident we shouldn't take too seriously.

Yeah, though this mechanism is simpler.

Does this allow us to get rid of architectures.nix? It's really annoying to have to extend that list with boilerplate when adding new architectures.

@@ -634,6 +634,12 @@ stdenvNoCC.mkDerivation {
+ optionalString ((targetPlatform ? gcc.cpu) && (isClang || !(targetPlatform.isDarwin && targetPlatform.isAarch64))) ''
echo "-mcpu=${targetPlatform.gcc.cpu}" >> $out/nix-support/cc-cflags-before
Copy link
Member

Choose a reason for hiding this comment

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

Also does this replace this mechanism for specifying the CPU?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it should replace it too, ideally.

Copy link
Member Author

@RossComputerGuy RossComputerGuy May 22, 2024

Choose a reason for hiding this comment

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

Yes, I just didn't strip out the way things are done with gcc.tune and gcc.cpu because I didn't want to break too much. My initial goal was to just get this mechanism to work.

Copy link
Member

@Ericson2314 Ericson2314 May 22, 2024

Choose a reason for hiding this comment

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

Unlike getting rid of architecture.nix, I am fine saving this for a follow-up PR :).

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, that was my plan. Maybe add a couple warnings that the file is up for removal so that we can give people a buffer

@Ericson2314
Copy link
Member

Does this allow us to get rid of architectures.nix? It's really annoying to have to extend that list with boilerplate when adding new architectures.

Yeah bottom line is I want a unified way of doing things. If we are doing this, then I think it's a hard requirement that is replaces architecture.nix. Having multiple ways of specifying things was exactly the sort of thing that the big lib/systems reorg I did was supposed to solve --- I don't want it creeping back.

@RossComputerGuy
Copy link
Member Author

This seems close to https://github.com/NixOS/nixpkgs/blob/master/lib/systems/architectures.nix ? My thought at the time with that is that I don't really know what the difference between an instruction set architecture and a "sub instruction set architecture", except for rather arbitrary conventions about what goes in the target triple or not, which I view as sort of historical accident we shouldn't take too seriously.

Yeah, though this mechanism is simpler.

Does this allow us to get rid of architectures.nix? It's really annoying to have to extend that list with boilerplate when adding new architectures.

Yes. This is a simpler alternative so we probably could deprecate it and remove it. This method only requires a couple lines to be changed and models which aren't added to the list could just have all the info specified as an attribute set instead of a string.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 15, 2024
@@ -25,7 +25,24 @@ let
inspect = import ./inspect.nix { inherit lib; };
platforms = import ./platforms.nix { inherit lib; };
examples = import ./examples.nix { inherit lib; };
architectures = import ./architectures.nix { inherit lib; };

architectures = throw "lib.systems.architectures was deprecated and replaced via the new cpuModel attribute";
Copy link
Member

@AndersonTorres AndersonTorres Sep 21, 2024

Choose a reason for hiding this comment

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

Deprecated has a specific meaning of "still works but we will remove in the future".
By using a throw, it no longer works.

Suggested change
architectures = throw "lib.systems.architectures was deprecated and replaced via the new cpuModel attribute";
architectures = throw "architectures was replaced by `cpuModel` attribute";

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, changed.

@RossComputerGuy RossComputerGuy force-pushed the feat/cpumodel branch 2 times, most recently from d567c24 to 5ac21f8 Compare September 22, 2024 02:06
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 12, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 11, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2024
@RossComputerGuy
Copy link
Member Author

Closing in favor of #376197

@RossComputerGuy RossComputerGuy deleted the feat/cpumodel branch January 23, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: lib The Nixpkgs function library 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 6.topic: zig 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CPU model to target platform
8 participants