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

flakes: move system to top-level of outputs #6776

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nrdxp
Copy link
Contributor

@nrdxp nrdxp commented Jul 9, 2022

Motivation

There has been a lot of discussion around re-working the way systems are handled in the flake output schema. Some of the suggested modifications are a bit contentious and not everybody seems to agree with how to best handle this.

I wanted to suggest a possible modification that I believe is sane and minimal, while also leaving the door open for other solutions such as #6773 or something similar to build on top of it.

All this PR aims to achieve is to move the expected location for system outputs to the toplevel of outputs, e.g. outputs.<system>.packages instead of outputs.packages.<system>.

To me, it just makes more sense to have a single attribute for each system rather than having several different <system> attributes nested in various outputs.

TODO:

I decided to open as a partially complete draft to collect some feedback on whether or not this is a desired approach before going through the trouble of updating all the docs.

Since flakes are currently considered unstable, I have not maintained backwards compatibility for the previous schema, but if this would be desired I can easily do so.

  • modify the cli commands to look for outputs in the new location
  • modify nix flake check (and others?) logic to account for the new schema (needs test fixes)
  • update relevant documentation

nrdxp added 2 commits July 9, 2022 13:00
Also improve error logging by incooperating valid systems as
suggestions.
@blaggacao
Copy link
Contributor

blaggacao commented Jul 9, 2022

This is better, future-proof ergonomics:

https://divnix.github.io/std/reference/conventions.html#top-level-system-scoping-of-outputs

This allows us to uniformly select on the current system and forget about it for most of the time.

If we don't do this we always will visibly have to deal with system at lower levels. That practice in itself is an unnecessary hit in UX, especially for non-nix or non-cross-compilation experts.

Evidence is flake-utils and things like (at the domain level, where system doesn't belong — having a more implicit transport of system for cross-compilation at the domain level is more appropriate):

{ inputs, system}:
{
  packages = inputs.packages.${system};
}

@tomberek
Copy link
Contributor

tomberek commented Jul 10, 2022

This is something I’ve thought about and brought it up for discussion a few weeks ago. I’m not totally convinced, though it is very handy to re-order the schema in this way.

A drawback is that the top-level attribute path string is no longer the “type” of the values contained, but rather is in “the first non-system attrpath string”. Consistency would suggest something like “outputs.no-system.overlays” so the pattern can remain “outputs.system.type”. But that can run into odd UX inconsistencies and seems wrong for the no-system case.

Another difference here is that it would make distinguishing these better on the command line:

  1. packages.x86_64-linux.foo
  2. apps.x86_64-linux.foo

because this forces one to be specific about the system when one only wants to be specific about the type. On the other hand, being specific about the system may be less common than the type?

To me, it just makes more sense to have a single attribute for each system rather than having several different attributes nested in various outputs.

This is probably best case for the change; that it is a common parameter between several types, not an independent sub-type of them.

@blaggacao
Copy link
Contributor

blaggacao commented Jul 10, 2022

On the other hand, being specific about the system may be less common than the type?

Precisely. And in general, having system as the root selector, current system would work on the CLI for (yet) unkown types. Currently, this doesn't work.

outputs.no-system.overlays”

In my experience, especially with lib the line between no-system and with-system is often just an IFD or such away. So in at least some subset of these use cases, having that distinction makes the overall UX worse: you start as no-sytem only to realize that you need to refactor everything.

Since function calls are memoized, thus there is no penalty of just ignoring the system input in the no-system case.

In my experience with std so far, this pattern has made things a lot easier, since most of the time you don't even care or want to deal with system (in the std-served use cases, at least).

@aakropotkin
Copy link
Contributor

While I really do understand why this helps ergonomics for the existing CLI tools; this has a dramatic effect on outputs which are intentionally system independent.

If a change like this is considered I urge contributors to preserve the ability to create arbitrary outputs like lib without splitting their cached eval or otherwise polluting it with system.

@blaggacao
Copy link
Contributor

preserve the ability to create arbitrary outputs like lib without splitting their cached eval or otherwise polluting it with system.

Absolutely!

However, allow me one remark on lib. In most projects I've been working so far at some point there is lib and then there is pkgs-lib. Any separation of the two has always been artificial.

My worst experience to that regard had been divnix/DevOS, where this differentiation caused unnecessary pain a lot of the time and as far as I remember (has been a while).

If system as an input argument is ignored, afaik memoization would make sure this has no double spending cost under any circumstances during a single eval.

Just my 2 cents based on my experiences, so far.

I'd be happy if we could manage to revive this discussion.

@stale stale bot added the stale label May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: To discuss
Development

Successfully merging this pull request may close these issues.

4 participants