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

Multi-arch support for hosts(nixosConfigurations) #161

Merged
merged 2 commits into from
Mar 19, 2021

Conversation

Pacman99
Copy link
Member

@Pacman99 Pacman99 commented Mar 14, 2021

fixes #72

also related, #125

This allows users to set nixpkgs.system to any architecture exported by the nixpkgs flake and nixpkgs.pkgs will be set to that system.

I also added multiPkgs as a special arg and made nixpkgs.pkgs a default. So if someone wanted to do crossSystem builds or anything else with pkgs, that is also an option(eg. mobile-nixos!!).

### root password is empty by default ###
imports = suites.base;

nixpkgs.system = "aarch64-linux";
Copy link
Member Author

Choose a reason for hiding this comment

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

This entire file is probably unnecessary and I could just add that line to the hosts README. But it is nice for testing the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this file, I would write a NixOS integration test similar to the profilesTest

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 a test would be a good idea, I'll try to add one.
But we still need to communicate to users that devos will respect nixpkgs.system without them having to dig through hosts/default.nix.

Copy link
Member Author

@Pacman99 Pacman99 Mar 15, 2021

Choose a reason for hiding this comment

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

Will nix flake check fail if the tests you wrote were exported to all systems? I was thinking of moving the checks output to under systemOutputs so the tests also have multi-arch support. I'm also trying to reduce the use of the one instantiated x86_64 pkgs above nixosConfigurations.

Copy link
Member Author

@Pacman99 Pacman99 Mar 15, 2021

Choose a reason for hiding this comment

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

well I guess my question is if nix flake check tries to go through the checks for all systems.

Copy link
Member Author

@Pacman99 Pacman99 Mar 15, 2021

Choose a reason for hiding this comment

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

yup just tested it, looks like it does, and now that I think about it, I don't think checks is even a multiarch output..
NVM forgot that I created a aarch64 host. So I think I can move the checks output under systemOutputs to simplify it.
But that raises another issue, nix flake check will fail when a user has an aarch64 host. So we probably have to filter out configurations when building the checks output.

I will try to include multi-arch checks as part of this 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.

Dropped nixos-a64.nix. I'm not sure how to write a test specifically for multi arch. I might try to figure it out later, but I don't think its completely necessary. And mulltiarch checks helps too.

@Pacman99 Pacman99 force-pushed the multiarch branch 3 times, most recently from 40f677c to 3f71a5d Compare March 14, 2021 20:47
@Pacman99 Pacman99 changed the title Multi-arch support Multi-arch support for hosts(nixosConfigurations) Mar 14, 2021
Comment on lines +73 to +76
deployHosts = nixos.lib.filterAttrs
(n: _: self.nixosConfigurations.${n}.config.nixpkgs.system == system) self.deploy.nodes;
deployChecks = deploy.lib.${system}.deployChecks { nodes = deployHosts; };
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't currently working. For some reason nix flake check evaluates all systems starting with 'x86_64'. Including x86_64-darwin, which now fails because the related nixpkgs doesn't evaluate on x86_64-linux.

Any ideas on what to do for this @nrdxp?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to debug this and I can't understand why nix flake check evaluates x86_64-darwin checks. I would assume that it would only build the checks for the current system.

Copy link
Member Author

@Pacman99 Pacman99 Mar 16, 2021

Choose a reason for hiding this comment

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

Ohh I see it tries to evaluate all systems which normally shouldn't be an issue, but because of the patched-nixpkgs it ends up using IFD. This is a known issue NixOS/nix#4265.

The optimal solution would be to avoid IFD in checks, but thats not possible right now because of the specialArgs issue.

And I believe switching to nixos-unstable will fix this.

@Pacman99 Pacman99 force-pushed the multiarch branch 2 times, most recently from 573c260 to 9ab19c4 Compare March 15, 2021 15:50
@Pacman99
Copy link
Member Author

Also switched to _module.args here, I think thats more correct.

@Pacman99 Pacman99 force-pushed the multiarch branch 3 times, most recently from f34aab5 to 22f2d42 Compare March 16, 2021 03:54
@Pacman99
Copy link
Member Author

Pacman99 commented Mar 16, 2021

dropped multiPkgs as a module arg in favor of #164. This is a rare use case anyways, and self.packages can be used instead.

@Pacman99
Copy link
Member Author

Fixed merge conflicts in latest push

@blaggacao
Copy link
Contributor

And following what you said, perhaps you could add a pkgs-lib to the pkgs folder. That would would get multi-arch support too.

While starting to work on #166, would it be possible to create a folder ./lib/pkgs-lib that does depend on packages, but still lives within the ./lib folder.

The reason is that I think ./lib token face value interpretation should still prime over ./lib as a dependency root, which to the average user is a little more subtle concept.

Hence the average user wold certainly wonder why a library function (regardless of it's dependencies) does not reside in a top level folder that is called precisely library.

It's just an UX thing, but I think devos cares.

@Pacman99
Copy link
Member Author

I'm still against passing pkgs to lib/default.nix. It is consistent to use pkgs/pkgs-lib with the way nixpkgs does it so I don't see how it would be surprising or bad UX.

If the lib/pkgs-lib folder is imported in a seperate line in flake.nix thats ok. The issue right now is lib is not a multi arch output, because it should just be pure nix functions. So any function written with the pkgs passed to lib will only run on one architecture..

@blaggacao
Copy link
Contributor

If the lib/pkgs-lib folder is imported in a seperate line in flake.nix thats ok.

That would have been my idea. I don't feel the nixpkgs's own top level dichotomy is particularly lucky. ./pkgs is such a hodgepodge of all sorts of things. That makes it actually harder for beginners to cut through. It would be nice if we could rather git ./pkgs a razor sharp mental profile.

The issue right now is lib is not a multi arch output, because it should just be pure nix functions. So any function written with the pkgs passed to lib will only run on one architecture..

I'm goging to anticipate this for #166

@Pacman99
Copy link
Member Author

That is a good point, but I still think proper multi arch support is more important. Is it alright if I still remove it in this PR, since theres no function atm that needs it. And we can figure out how to reintegrate it properly when we need it.

@blaggacao
Copy link
Contributor

Perfect!

@nrdxp
Copy link
Collaborator

nrdxp commented Mar 17, 2021

That is a good point, but I still think proper multi arch support is more important. Is it alright if I still remove it in this PR, since theres no function atm that needs it. And we can figure out how to reintegrate it properly when we need it.

Whe it's ready feel ready.

bors delegate+

@bors
Copy link
Contributor

bors bot commented Mar 17, 2021

✌️ Pacman99 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@Pacman99
Copy link
Member Author

This will fail right now because of multi arch checks. So its blocked by #192.

@Pacman99 Pacman99 force-pushed the multiarch branch 6 times, most recently from f664eae to f1abf9c Compare March 19, 2021 01:43
@Pacman99
Copy link
Member Author

I updated this PR so its no longer blocked by #192. Until we have proper multi-arch tests, the tests are now only imported if the system is x86_64-linux.

@nrdxp
Copy link
Collaborator

nrdxp commented Mar 19, 2021

Been out all day. Super exhausted, but I'll have a proper review of this tomorrow. Good job.

Copy link
Collaborator

@nrdxp nrdxp left a comment

Choose a reason for hiding this comment

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

This is all but ready. Just fix my small nitpick in the docs and we can go ahead and merge it.

hosts/README.md Outdated
@@ -25,6 +25,9 @@ that you intend to use on your machine.
Additionally, this is the perfect place to import anything you might need from
the [nixos-hardware][nixos-hardware] repository.

Set `nixpkgs.system` to the architecture of this host, default is "x86_64-linux".
Copy link
Collaborator

@nrdxp nrdxp Mar 19, 2021

Choose a reason for hiding this comment

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

I would make this a note. As the continuation from the previous paragraph doesn't really flow well. Plus, we want it to stand out so users don't overlook this information.

so Just

Note:

Set nixpkgs.system to the architecture of this host, default is "x86_64-linux". ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, that looks better. Can you look at the update. I followed the format of notes from the docs folder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Almost there. The format I've used throughout the docs so far includes blockquotes and a level 5 header.

> ##### _Note:_
> Some cool note.

or for multiple notes:

> ##### _Notes:_
> * A note
> * Another note

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok got it, should be fixed now

Copy link
Member Author

Choose a reason for hiding this comment

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

Missed the blockquotes. I think its good now.

@Pacman99 Pacman99 force-pushed the multiarch branch 3 times, most recently from 875a4e0 to 47a7aae Compare March 19, 2021 19:06
Copy link
Collaborator

@nrdxp nrdxp left a comment

Choose a reason for hiding this comment

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

Good to go.

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 19, 2021

Build succeeded:

@bors bors bot merged commit c050027 into divnix:core Mar 19, 2021
@Pacman99 Pacman99 deleted the multiarch branch March 19, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARM aarch64 Support
3 participants