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

Return to factoring and comments on flake once major features have all been added #27

Closed
cleeyv opened this issue Jul 27, 2023 · 4 comments
Labels
documentation Improvements or additions to documentation infra Work on Ngipkgs itself, and related infrastructure maintenance Cleanup, refactoring, improving discoverability, tending to continuos integration

Comments

@cleeyv
Copy link
Collaborator

cleeyv commented Jul 27, 2023

The "major features" in the title primarily refers to ongoing work on CI (#24) , as well as future work on nixos-tests (#28), support for public VM (cloud) deployments (#22), and import of Ngipkgs into existing NixOS configs (#21).

I'm making this issue in order to not lose track of a good conversation that happened in a PR today:
#26

Following from that conversation, I just added some additional comments into the flake:
ac50c3b

The comment I'm least confident about, and therefore would prioritize returning to, is the explanation of the location of nixosConfigurations as the workaround for nixos-container parsing of attribute paths. This is my rewording of the original comment by @fricklerhandwerk that was removed in PR #26 and prompted this work. Relately, as I say in that PR, I think would also be good to have more structural commentary on the reasons for, and differences between, buildOutputs and checkOutputs.

In the end, the structuring of the flake we've arrived at organically through debugging could end up not being relevant anymore and could be refactored into something more logical but I would wait until features are done being added before considering that.

@cleeyv cleeyv added documentation Improvements or additions to documentation maintenance Cleanup, refactoring, improving discoverability, tending to continuos integration infra Work on Ngipkgs itself, and related infrastructure labels Jul 27, 2023
@jfly
Copy link
Contributor

jfly commented Jul 27, 2023

Just to explain the names buildOutputs and checkOutputs (and why they're probably not the right names, or even necessarily the right long term structure):

We (@ngi-nix/fungus) were under the (incorrect) understanding that the only reason for the attrset merge in flake.nix was to carefully only define hydraJobs and checks for x86_64-linux, because defining them for all systems would break CI (#24 for a good explanation). So we called it checkOutputs because we thought this was entirely about CI. We sort of glossed over the fact that there's a nixosConfiguration defined here as well. My understanding now is that that nixosConfiguration has nothing to do with checks, and therefore probably doesn't belong in something named checkOutputs.

I suspect this would all get a lot cleaner if we could start defining checks for all systems, but that would require addressing #24.

@cleeyv
Copy link
Collaborator Author

cleeyv commented Aug 23, 2023

The @ngi-nix/algae team has done a great job on a major refactor of the flake for ngipkgs as part of their work on the pretalx service in #38. @fricklerhandwerk and I joined one of their mobs yesterday to better understand what they had done, and to provide some context for the earlier iterations of the flake.

In addition to removing the checkOututs and buildOutputs attribute sets, they have also successfully used overlays to resolve a deeper problem that @fricklerhandwerk and I had worked on in the early days of ngipkgs with getting the pkgs set included in modules.

The outputs of the new flake are organized into three different attribute sets, sorting outputs based on their relationship to flake-utils, what system they run on, and whether they need a system defined at all (all of this is more clearly documented in the comments of the flake). For future reference: they said that if the entire repo switched to linux-only then sets 2 and 3 could be combined, and set 1 would become redundant. However, while there is still the possibility of future support for different systems (such as #24 for darwin) this breakdown into different attribute sets provides a clear foundation for that work.

For their work on pretalx they also introduced a new output and and associated repo directory for nixosTests, which is a substantial advancement for #28, and they also introduced a few different ways for the outputs of the flake to be used in other flakes as needed for #21. I'm also going to write updates on these two issues.

Even though not all the originally listed features have been added yet, I think this work addresses the core need I had in mind when I created this issue so I'm going to close.

@cleeyv cleeyv closed this as completed Aug 23, 2023
@jfly
Copy link
Contributor

jfly commented Aug 23, 2023

Nice. The refactor looks great to me!

@lorenzleutgeb
Copy link
Member

The outputs of the new flake are organized into three different attribute sets, sorting outputs based on their relationship to flake-utils, what system they run on, and whether they need a system defined at all (all of this is more clearly documented in the comments of the flake). For future reference: they said that if the entire repo switched to linux-only then sets 2 and 3 could be combined, and set 1 would become redundant. However, while there is still the possibility of future support for different systems (such as #24 for darwin) this breakdown into different attribute sets provides a clear foundation for that work.

Minor correction: Looking at

ngipkgs/flake.nix

Lines 59 to 64 in 0893452

# We merge three attribute sets to construct all outputs:
# 1. Outputs that are invariant in the system architecture
# via `flake-utils.lib.eachDefaultSystem`.
# 2. Outputs that are specific to a system architecture
# (as of 2023-08-22, only `x86_64-linux`).
# 3. Outputs that are not tied to any system at all.
actually groups (1.) and (2.) would merge: The argument system that (1.) takes would become constant, i.e. system = "x86_64-linux", and equal to the variable linuxSystem = "x86_64-linux" relevant for the second group. Thus, these two blocks could be merged. At this point, one might also think about abandoning separation into blocks, since system = "x86_64-linux" would be everywhere, and it's not as important anymore to think about the distinction:

Is intended to work for which system?
any*only x86_64-linux
Depends on system?yes12
no3

*: This quantifies over all "default systems".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation infra Work on Ngipkgs itself, and related infrastructure maintenance Cleanup, refactoring, improving discoverability, tending to continuos integration
Projects
Status: mobleted
Development

No branches or pull requests

3 participants