[READY] nix.treewide: remove flake-parts #800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Create a new home for nixos modules. Moved most of them over. Gated them all behind enable flags.
Create a new home for nixos configurations. Moved them all over.
Move pkgs to packages and follow the by-name schema from upstream. Create overlays for all the packages and expose them in the flake outputs. Use the overlays to create an exposed legacy-packages output. Had to modify the devshells and test for now because they were causing failures.
Create a new home for devShells. Moved them all over and created new individual ones for smaller package sets.
Flatten the flake inputs. Makes it easier to see all inputs at once when they are sorted.
Change the legacyPackages to us nixpkgs and not nixpkgs-unstable. Unless for good reason, when using nixpkgs from inputs, it should use the nixpkgs input which can be rebound to any other input. But it stays consistent across its use in the repository.
Since legacyPackages is now bound to the nixpkgs input, it can be used in other places like devShells. If another package set is needed, it can be brought into scope but the default one bound to `pkgs` should come from legacyPackages as it will have the necessary local overlays.
Created a new home for checks. Moved them all over.
flake-parts is no longer necessary and has been removed.
Removing leftover directories from the flake-parts migration.
An old file from the flake-parts migration was still being referenced and was removed near the end of the clean up. The `loghost` NixOS configuration has now been split so the test can refer to the since configuration as before and the host can still be built as normal.
|
small PR |
Missed one more file from the flake-parts migration.
Added the locally provided packages back as flake outputs. This is how they are accessed and build in github CI.
djacu
changed the title
While removing flake-parts, this input got bumped and is changing the closures for packages and nixos systems. One of the goals during the migration from flake-parts was to not have the closures change, effectively a no-op. This input is being pinned so closures can be compared between this branch and master to verify closures have not changed.
|
A script has been written to compare the closures of all the packages and nixos systems listed below. > cat closure-compare.sh
my_array=(
"massflash"
"serverspec"
"scaleInventory"
"nixosConfigurations.bootstrapImage.config.system.build.toplevel"
"nixosConfigurations.cache.config.system.build.toplevel"
"nixosConfigurations.coreMaster.config.system.build.toplevel"
"nixosConfigurations.coreSlave.config.system.build.toplevel"
"nixosConfigurations.devServer.config.system.build.toplevel"
"nixosConfigurations.hypervisor1.config.system.build.toplevel"
"nixosConfigurations.hypervisor2.config.system.build.toplevel"
"nixosConfigurations.loghost.config.system.build.toplevel"
"nixosConfigurations.massflash.config.system.build.toplevel"
"nixosConfigurations.monitor.config.system.build.toplevel"
"nixosConfigurations.signs.config.system.build.toplevel"
)
currentBranch="djacu/remove-flake-parts"
for attrPath in "${my_array[@]}"; do
echo "$attrPath"
git switch -q master
nix eval .\#"$attrPath".drvPath
git switch -q "$currentBranch"
nix eval .\#"$attrPath".drvPath
echo ""
doneHere is the output from running the script as of commit 960ce8a |
|
Reviewing the
The hostname differs because it was previously unset and defaults to The users differ because of the addition of The packages differ most likely because of the addition of |
|
The |
|
|
kylerisse
reviewed
Nov 26, 2024
There was a problem hiding this comment.
@djacu Here is my testing output. nix flake check worked but had the warnings below. nix develop also worked. Not sure if those warnings are important. I included a nix-info output as it might be something related to 24.11 or my version of cppnix. @sarcasticadmin will likely have more comprehensive testing but so far it doesn't appear anything broke at a high level. Please let me know if there's anything specific you want tested.
kylerisse@watson ~/g/s/g/k/scale-network (djacu/remove-flake-parts) [SIGHUP]> nix flake check
warning: unknown flake output 'formatterModule'
warning: unknown flake output 'library'
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: In a derivation named 'neovim-unwrapped-0.9.5', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks.<output>.disallowedRequisites' instead
warning: The check omitted these incompatible systems: aarch64-darwin, aarch64-linux, x86_64-darwin
Use '--all-systems' to check all.
kylerisse@watson ~/g/s/g/k/scale-network (djacu/remove-flake-parts)> nix shell nixpkgs#nix-info --command nix-info -m
- system: `"x86_64-linux"`
- host os: `Linux 6.11.7, NixOS, 24.11 (Vicuna), 24.11.20241116.057f63b`
- multi-user?: `yes`
- sandbox: `yes`
- version: `nix-env (Nix) 2.24.10`
- channels(root): `"nixos"`
- nixpkgs: `/nix/store/idq1bmwwy9rlkc21hccvx42wlwpxsx1f-source`
kylerisse@watson ~/g/s/g/k/scale-network (djacu/remove-flake-parts)> git lsha
960ce8a26aeae941cf13828d3464d38ff77a8acd
kylerisse@watson ~/g/s/g/k/scale-network (djacu/remove-flake-parts)> nix develop
[nix-flakes scale-network] $ exit
exit
|
@kylerisse thanks for review the changes. @sarcasticadmin and I aren't seeing the warnings about neovim-unwrapped. Here is the output for The neovim-unwrapped warning seems to be an upstream issue. I found a downstream issue related to it here. nix-community/nixvim#2037 The other warnings I got are nothing to be concerned about. unknown flake outputs are because we have flake outputs that are not part of the default flake schema. The omitted systems are because we only ran checks that are native to the host. |
|
@kylerisse are you using neovim-unwrapped in your system? |
devServer and hypervisor1 had configurations that were different from how they were on master before the flake-parts migration. Fixed these and checked the closure differences with @sarcasticadmin. Now the only apparent difference besides dumb system packages strings is coreMaster's bind serial number which is expected to be different.
|
Not sure what the argument for removing flake parts is. I don't like to see code implementation spat out in front of my face. I say that as someone who used the genAttrs pattern for years before finding that flake.parts was worth using, compared to things like flake-utils which still spit the implementation out in your face often. I think you'll re-introduce flake.parts before long, once you realise that you need the module system for the scale of this repo. |
|
|
Yeah awesome, it's about what works for the people doing the active work on the repo. I can help re-introduce it again in the future, if the need arises, which I think it will. |
|
Thanks man. We'll definitely be calling on you for your expertise 🙏 |
had Here's more verbose output: Like I said initially, they're just warnings and nothing fails. I'm just perplexed that it's even related at this phase as it should be pretty sandboxed no? and neovim isn't used for SCaLE AFAIK. Nothing I'm worried about or want to spend further time on though if you two say it's OK. Let me know if you need further testing or info though. |
kylerisse
approved these changes
Nov 27, 2024
|
Thanks for digging in more @kylerisse. Honestly, I was just grasping at straws when I asked about your system configuration. But I think I figured it out. It is at least partially related to the version of Nix that is being used. When I ran |
sarcasticadmin
changed the title
|
Thanks for the reviews @MatthewCroughan and @kylerisse sounds like were all the same page and this restructuring its a big improvement from where we were. @djacu thanks again for all the effort, Im excited to have this land in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Removes flake parts and restructures the nix sections of the repository.
Previous Behavior
Absolute chaos
New Behavior
Bringing structure and order to the architecture of the nix code.
The hope is that this PR is effectively a no-op and the packages, NixOS configurations, and checks are the same.
Testing in comments below found that the only remaining differences are NixOS system closures revolving around:
berkhanbeing added as a user to all systemsberkhanbeing added (e.g. a single new instance of bash showed up) OR system packages being strings and order had changed for no reason (usingjdconfirmed that the package set was identical in the few cases that were tested)Tests
nix flake checkLots more see below