Filter away unset output attributes #59

L-as · 2022-09-28T19:04:09Z

…hello`)

This in addition also strips away unset attributes set using flake.

The core issue with IFD in this context, is that we want to strip away unused attributes. AFAICT, flake-parts did this in a buggy way before. Removing this entirely while fixing support for IFD was easier, but to keep support for filtering away unset attributes, a bit more complexity was needed.

The issue is that we can't use null as a special value to mean "unset". The reason is that while filtering over allSystems.${system}, checking whether packages has been set, doing null == packages will cause packages, an attribute set, to be evaluated to WHNF. WHNF for an attribute set entails evaluating the keys, and if the keys are generated using IFD, then this will fail, even if we know that no matter what the key is, this check will return false.

If the Nix evaluator is ever fixed, this feature can be somewhat cleaned up by using null checks rather than checking isDefined.

To make use of this feature, you can not simply do

packages = import drv;

You must do

packages = mkOverride 100 (mkOrder 1000 (import drv));

The reason is that the module system internally will try to check ._type, but this of course will cause all of the other keys to also be evaluated.

This would also be unnecessary if the Nix evaluator were fixed.

Closes #57

…hello`) This in addition also strips away unset attributes set using `flake`. The core issue with IFD in this context, is that we want to strip away unused attributes. AFAICT, flake-parts did this in a buggy way before. Removing this entirely while fixing support for IFD was easier, but to keep support for filtering away unset attributes, a bit more complexity was needed. The issue is that we can't use `null` as a special value to mean "unset". The reason is that while filtering over `allSystems.${system}`, checking whether `packages` has been set, doing `null == packages` will cause `packages`, an attribute set, to be evaluated to WHNF. WHNF for an attribute set entails evaluating the _keys_, and if the keys are generated using IFD, then this will fail, even if we know that no matter what the key is, this check will return false. If the Nix evaluator is ever fixed, this feature can be somewhat cleaned up by using null checks rather than checking `isDefined`. To make use of this feature, you can not simply do ```nix packages = import drv; ``` You must do ```nix packages = mkOverride 100 (mkOrder 1000 (import drv)); ``` The reason is that the module system internally will try to check `._type`, but this of course will cause all of the other keys to also be evaluated. This would also be unnecessary if the Nix evaluator were fixed. Closes hercules-ci#57

roberth · 2022-09-28T19:40:31Z

Good analysis of the strictness issue.

to keep support for filtering away unset attributes, a bit more complexity was needed.

The code was purely wishful thinking that I've glossed over. It didn't work, but I don't think it was a problem. By adding more filterAttrs, one layer higher, we'd introduce more strictness once again. I don't think mkIf will help us out either, because that only really works with attrsOf, whereas flake-parts only uses lazyAttrsOf (again to avoid filterAttrs-like strictness-inducing behavior).
Maybe there's something I'm not seeing? If so, could you give me a concrete example of a real world problem that is solved by the extra filterAttrs?

This would also be unnecessary if the Nix evaluator were fixed.

I don't think the operational semantics were specified to include laziness at such a fine granularity, so calling this change (whatever it is) a fix would be an exaggeration.

L-as · 2022-09-28T21:00:06Z

I'm using this code in practice here https://github.com/mlabs-haskell/mlabs-tooling.nix/blob/las/work/flake.nix#L156

The extra filterAttrs that runs on the output of the module evaluation just evaluates all top-level attributes to WHNF, and I think that's fine since that should be system-independent in the first place.

The older code not working wasn't a problem (ignoring IFD), but it seemed to me that you intended to not set attributes that the user didn't mean to set, and I think that would be a very useful features. For most flakes, exposing an empty nixosModules makes little sense.

roberth · 2022-09-28T21:31:30Z

you intended to not set attributes that the user didn't mean to set

While that was my intent, I am not convinced that it is important.

It wouldn't be the first time that a project starts with some bad, redundant code.

For most flakes, exposing an empty nixosModules makes little sense.

The other day, I answered a strictness question from someone who was doing something like options = f myPkgs. I'd never thought of doing that, so to me it seems like a question of when rather than whether someone will make nixosModules and packages mutually recursive by accident. It's crazy until it's not.

I agree on the fact, but I can't act on it; especially because the infinite recursion errors are as bad as they are, and when the goal is for the project to be general.

roberth · 2022-09-28T21:35:05Z

I've updated the title to (what I believe to) better reflect the effect of the proposed change. That is, assuming #58 did its job for #57.

roberth

I don't think I can make this change happen, but I hope you can appreciate the ideas in my comments.

roberth · 2022-09-28T21:38:23Z

modules/perSystem.nix

+ # - Doesn't handle undefined options in submodules.
+ # - Doesn't handle undefined options that have a prefix, e.g. `_module.args`.
+ # TODO: Upstream a good version of this to Nixpkgs, probably as an extra result
+ # from `evalModules`, e.g. `result.configFiltered`.


We should be careful about adding attributes. They're not like methods that have no per-object overhead.

roberth · 2022-09-28T21:42:21Z

modules/perSystem.nix

+ # and only check whether they're defined. Hence we can't check for nullness,
+ # because that will evaluate a lot of stuff unnecessarily and cause errors
+ # with IFD. A more proper fix would perhaps be fixing the Nix evaluator to
+ # short circuit more, specifically, in an expression like `null != { ${x} = ...; }`,


This requires a Value to have an intermediate state between thunk and attrset, and it will cause make the != operator asymmetric. Neither is terrible in itself, but nix has to strike a balance. If you want to make this happens, please contribute to the test suite or other quality control measures such as combining boehmgc and c++ sanitizers.

roberth · 2022-09-28T21:42:44Z

modules/perSystem.nix

+ # short circuit more, specifically, in an expression like `null != { ${x} = ...; }`,
+ # `x` should not be validated, since it's a set regardless.
+ #
+ # In general though, Nix's evaluator sucks.


Not very constructive, is it?

L-as · 2022-09-28T23:05:10Z

I agree on the fact, but I can't act on it; especially because the infinite recursion errors are as bad as they are, and when the goal is for the project to be general.

I don't think the code here makes it any worse than it was before. isDefined is safe to use in this context AFAICT.

roberth · 2022-11-12T17:34:49Z

modules/apps.nix

+ mkIfNonEmptySet
+ (mapAttrs
+ (k: v: v.apps)
+ (filterAttrs
+ (k: v: v ? apps)
+ config.allSystems
+ ));


This technique makes attributes such as devShell.x86_64-linux.default depend on checks.aarch64-darwin. (To pick a highly unrelated example)

You can see this when you define

systems = [ "x86_64-linux" "aarch64-darwin" ]; perSystem = { system, ... }: { checks = if system == "aarch64-darwin" then throw "not ok" else {}; }

and then instantiate devShell.x86_64-linux.default.

Usually checks.<system> isn't too slow or fragile, but that's not an assumption we can make. Also strictness can lead to infinite recursions, which are horrible.

Thanks, yes, optimally this shouldn't happen.

roberth · 2022-11-12T18:11:49Z

What is your reason to filter attributes with empty value?

shanesveller · 2022-11-12T18:42:05Z

Not taking a stance on whether this should or shouldn't be handled upstream, but couldn't we already use the disabledModules syntax to locally omit any flake-parts content we don't intend to ever use? A pseudocode example might be:

disabledModules = ["nixosConfigurations.nix" "nixosModules.nix" "darwinModules.nix" "legacyPackages.nix"];

for a project-specific dev shell that doesn't make sense for it to deal in NixOS/Darwin modules or nixpkgs-style output trees?

L-as · 2022-12-20T00:05:03Z

@shanesveller That does seem possible, thanks for informing me. However, this still seems more ergonomic.

@roberth Because it blows up the size of the output and adds a bunch of unnecessary information to e.g. nix flake show. Any time someone wants to type some output attribute, it bloats up every flake using flake parts more.
Ideally, the output shouldn't be any different compared to not using flake parts.

L-as · 2023-03-03T12:19:52Z

The only remaining benefit of this is that the attributes are truly not there, meaning tab-completion in nix repl won't see them either. I'm not sure whether that's worth it.

roberth · 2023-05-08T20:34:53Z

nix repl is still a fair point, but I agree that it doesn't weigh up against the potential for infinite recursions or slower evals.

@shanesveller's suggestion still holds some potential. I think the nixos* modules could be moved to nixpkgs and perhaps some other modules could be made opt-in, like flakeModules.

L-as force-pushed the fix-for-ifd branch from 84f2ead to a8a2d70 Compare September 28, 2022 19:05

L-as mentioned this pull request Sep 28, 2022

Remove redundant null filters #58

Merged

roberth changed the title ~~Allow using IFD to generate attribute names (e.g. `package.${system}.…~~ Filter away unset output attributes Sep 28, 2022

roberth reviewed Sep 28, 2022

View reviewed changes

roberth reviewed Nov 12, 2022

View reviewed changes

roberth mentioned this pull request Jan 29, 2023

[RFC] set default for systems and use a better interface for exposing packages #110

Closed

srid mentioned this pull request Jan 31, 2023

feat: don't output empty systems #113

Closed

roberth mentioned this pull request Jan 31, 2023

nix flake show: Ignore empty attrsets NixOS/nix#7726

Merged

7 tasks

roberth added requires upstream change Issues that need a change in Nix, Nixpkgs, etc undecided Depends on postponed design decision labels Feb 1, 2023

roberth removed the requires upstream change Issues that need a change in Nix, Nixpkgs, etc label May 8, 2023

roberth closed this May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter away unset output attributes #59

Filter away unset output attributes #59

L-as commented Sep 28, 2022 •

edited

Loading

roberth commented Sep 28, 2022

L-as commented Sep 28, 2022

roberth commented Sep 28, 2022

roberth commented Sep 28, 2022

roberth left a comment

roberth Sep 28, 2022

roberth Sep 28, 2022

roberth Sep 28, 2022

L-as commented Sep 28, 2022

roberth Nov 12, 2022

L-as Dec 20, 2022 •

edited

Loading

roberth commented Nov 12, 2022

shanesveller commented Nov 12, 2022 •

edited

Loading

L-as commented Dec 20, 2022

L-as commented Mar 3, 2023

roberth commented May 8, 2023

Filter away unset output attributes #59

Filter away unset output attributes #59

Conversation

L-as commented Sep 28, 2022 • edited Loading

roberth commented Sep 28, 2022

L-as commented Sep 28, 2022

roberth commented Sep 28, 2022

roberth commented Sep 28, 2022

roberth left a comment

Choose a reason for hiding this comment

roberth Sep 28, 2022

Choose a reason for hiding this comment

roberth Sep 28, 2022

Choose a reason for hiding this comment

roberth Sep 28, 2022

Choose a reason for hiding this comment

L-as commented Sep 28, 2022

roberth Nov 12, 2022

Choose a reason for hiding this comment

L-as Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

roberth commented Nov 12, 2022

shanesveller commented Nov 12, 2022 • edited Loading

L-as commented Dec 20, 2022

L-as commented Mar 3, 2023

roberth commented May 8, 2023

L-as commented Sep 28, 2022 •

edited

Loading

L-as Dec 20, 2022 •

edited

Loading

shanesveller commented Nov 12, 2022 •

edited

Loading