`__cleanAttrs`: Clean packages that don't expose all internals #217243

roberth · 2023-02-19T23:30:51Z

Description of changes

What

Clean up the attribute zoo on packages, opt-in.

Why

Automatically exposing internals blurs the line between what's expected to be stable and what's not. Let's not do that.

How

Call derivationStrict instead of derivation. derivation is backed by derivationStrict anyway.

When you specify __cleanAttrs = true; in a mkDerivation call, you won't be exposing internals.

What else

This PR improves memory by some 1-2% and cpu time also improves.

Internals are still accessible via overrideAttrs, lib.inspectMkDerivationArgs and pkgs.inputDerivation.

This relates weakly to a proposed improvement to make Nix take outputs from package attrsets instead of store derivations, as both this PR and the proposal work towards a more accurate as an expression-level package concept.

Redefine outputs in terms of language-level "package", not necessarily store-level derivation (RFC-92, and multi-drv packages, docs) nix#6507

Things done

Artturin · 2023-02-21T04:11:24Z

Internals are still accessible via overrideAttrs, lib.inspectMkDerivationArgs and pkgs.inputDerivation.

In mkDerivation(finalAttrs: too?

winterqt · 2023-02-21T04:43:37Z

Do we want to make cleanAttrs opt-out any time soon (with perhaps defaults for allowing pname, version, and src to passthru), or will it take a lot of effort (do a lot of things in-tree depend on internal attrs)?

roberth · 2023-02-21T16:19:42Z

Internals are still accessible via overrideAttrs, lib.inspectMkDerivationArgs and pkgs.inputDerivation.

In mkDerivation(finalAttrs: too?

finalAttrs and prevAttrs behave as before, except finalAttrs.finalPackage is smaller, as is to be expected because it is defined to match the return value of mkDerivation.
Fixes for overrideAttrs (split from #201734) #211685 sets more precedent for extending finalAttrs with more special attributes is useful and/or necessary, so I can imagine adding the complete drvAttrs there, if not cost prohibitive for some reason. I think this would be an extra feature that does not have to be implemented here.

Do we want to make cleanAttrs opt-out any time soon (with perhaps defaults for allowing pname, version, and src to passthru), or will it take a lot of effort (do a lot of things in-tree depend on internal attrs)?

I don't think we'll do this anytime soon, if at all. Applying that change too soon would mean that we fail to pay attention to the actual usage of some packages' attributes, which would be bad for users. cleanAttrs may never be suitable for being enabled by default in mkDerivation, but a successor to mkDerivation may enable it by default or its only behavior. The existence of cleanAttrs provides an anchor point for conversations about such a successor, and helps with experimentation.

SuperSandro2000 · 2023-02-24T10:57:25Z

So we should document that it should never be set in any package in nixpkgs without strong reasoning?

pkgs/applications/misc/hello/default.nix

The implementation with dynamic attributes is a little weird, but it performs better by avoiding some attrset copying and sorting, `//`.

roberth · 2023-03-16T22:34:43Z

I think we can just make __cleanAttrs = true warn for one release

Then every package that wants to opt in would have to do it in the first release. I don't think that's a good approach, and the cost of "warn" is low enough.

I'd like to avoid blowing up the scope and merge this soon. It adds the mechanism, but avoids changing the policy, so that we can

start applying the solution on a small scale
start solving the engineering problem
gain data and experience to inform policy decisions

SuperSandro2000 · 2023-04-01T00:21:30Z

Making the interface of a package explicit helps with maintenance by reducing the surface area that a maintainer has to worry about. This also benefits users, as cleanAttrs starts the conversation about what the public interface should be. Furthermore, we can expect a further performance benefit from having smaller attrsets when applied at a larger scale.
For a single package, I don't see what specifics would make this reasoning stronger, making "strong reasons" a rather tough requirement.

For me this translates into more work and discussions. Sometimes I know that the solution I am drafting is hacky and I decided for myself that it is better than the available alternatives. I don't want to contact another person to expose some variable(s) for that though. I personally would want a setting to just disable cleanAttrs everywhere and give me access to everything, as I don't really care if something is marked as an interface or not.
Or do we now need to start doing reflections?

For example this would majorly break nix-update and complicate it.

https://github.com/Mic92/nix-update/blob/f7eba589cc99a0cb6f0bab298e194c7c5705be13/nix_update/eval.py#L106-L118

SuperSandro2000 · 2023-04-01T00:24:42Z

nixos/doc/manual/release-notes/rl-2305.section.md

@@ -165,6 +165,10 @@ In addition to numerous new and upgraded packages, this release has the followin

 - Pantheon now defaults to Mutter 42 and GNOME settings daemon 42, all Pantheon packages are now tracking elementary OS 7 updates.

+- `mkDerivation` does not have to leak implementation details anymore.
+
+  Passing [`__cleanAttrs = true`](https://nixos.org/manual/nixpkgs/unstable/#var-__cleanAttrs) to `mkDerivation` returns a minimal set of package attributes that can be extended via [`passthru`](https://nixos.org/manual/nixpkgs/unstable/#var-stdenv-passthru). This reduces confusion for package users and allows for confident use of non-standard attributes provided by a package. It also gives package authors more confidence in modifying the builder environment without breaking consumers of their package, and it is slightly more efficient.


I really don't like the wording of this. It encourages people to just set cleanAttrs for all their packages, only to later get feedback from people that they need x, y, z back.

How about this?

Suggested change

Passing [`__cleanAttrs = true`](https://nixos.org/manual/nixpkgs/unstable/#var-__cleanAttrs) to `mkDerivation` returns a minimal set of package attributes that can be extended via [`passthru`](https://nixos.org/manual/nixpkgs/unstable/#var-stdenv-passthru). This reduces confusion for package users and allows for confident use of non-standard attributes provided by a package. It also gives package authors more confidence in modifying the builder environment without breaking consumers of their package, and it is slightly more efficient.

Passing [`__cleanAttrs = true`](https://nixos.org/manual/nixpkgs/unstable/#var-__cleanAttrs) to `mkDerivation` returns a minimal set of package attributes that can be extended via [`passthru`](https://nixos.org/manual/nixpkgs/unstable/#var-stdenv-passthru). For new packages, this reduces confusion for package users and allows for confident use of non-standard attributes provided by a package. It also gives package authors more confidence in modifying the builder environment without breaking consumers of their package, and it is slightly more efficient. It may be applied to existing packages with great care, as other users may unknowingly rely on internals. To uncover such usages without breaking their code, you may first pass `__cleanAttrs = "warn"` instead.

That sounds better. 👍🏼

SuperSandro2000 · 2023-04-01T00:25:38Z

pkgs/applications/misc/hello/salve-mundi.nix

+  # We allow `hello.src` to be used in tests and examples, despite __cleanAttrs
+  passthru.src = finalAttrs.src;


IMO src should always be passed through. eg you sometimes want to (re-)build it and as I understand it, that would after that no longer be possible which would suck.

src -> internals.src should be fine for individual "override" packages you write.

There is no general way to get all sources in using just the expression language, so we don't lose the capability to generically retrieve all sources, because we never had that in the language. Simply put, not all packages have a src, and not all sources appear in src, and you can hide "anything" in a dependency of a derivation a string context.
This task is better achieved by analyzing the .drv graph, you can find all the derivation input sources (file references and builtin fetcher results) and fixed output derivations. You may even do this from within a derivation.

roberth · 2023-04-01T14:32:11Z

For me this translates into more work and discussions. Sometimes I know that the solution I am drafting is hacky and I decided for myself that it is better than the available alternatives. I don't want to contact another person to expose some variable(s) for that though.

You'll still be able to do this, by using attributes such as .internals.src. If you don't think your use case should be explicitly supported by the package, you don't need to do anything.

Also if you're very confident about your use case, you could just open a PR. It doesn't have to some sort of drawn out discussion.

I personally would want a setting to just disable cleanAttrs everywhere and give me access to everything, as I don't really care if something is marked as an interface or not. Or do we now need to start doing reflections?

What do you mean with reflections?

I'm not in favor of such a global flag, because it is not necessary and it will make some expressions that do or do not use the flag incompatible. (Configuration should always be a last resort)

For example this would majorly break nix-update and complicate it.

https://github.com/Mic92/nix-update/blob/f7eba589cc99a0cb6f0bab298e194c7c5705be13/nix_update/eval.py#L106-L118

It'd be fine for nix-update to peek inside the internals and then carry on as it did.

It would be nicer if the language integrations and nix-update agreed on some small but general interface, such as a package attribute, so that nix-update isn't coupled to each of the language integrations. More concretely, if the language integrations provided a default updateScript attribute, nix-update would be trivial. That's a separate project though that I don't really want to delve into here, but I think it shows that internals highlights the places where the design could be improved.

SuperSandro2000 · 2023-04-01T21:09:07Z

You'll still be able to do this, by using attributes such as .internals.src.

Totally forgot about that. 😓 Then this should be okay merge.

It would be nicer if the language integrations and nix-update agreed on some small but general interface, such as a package attribute, so that nix-update isn't coupled to each of the language integrations. More concretely, if the language integrations provided a default updateScript attribute, nix-update would be trivial. That's a separate project though that I don't really want to delve into here, but I think it shows that internals highlights the places where the design could be improved.

I think nix-update exactly was build out of that problem that no such general attribute existed.

infinisil

I'm a bit worried that people will start using __cleanAttrs = true on existing packages, breaking a lot of things without warnings downstream (and for every user complaining about an attribute gone missing there's 10 more in the back that never reported it). We'd have to trust reviewers to not merge such changes. Better of course would be CI, but that adds a lot of complexity.

Other than that this looks pretty good to me, I like the salve mundi!

infinisil · 2023-04-03T17:45:17Z

doc/stdenv/stdenv.chapter.md

+
+New packages may pass `__cleanAttrs = true;` to `mkDerivation`, so that it will return a minimal set of package attributes, which package authors can extend via [`passthru`](#var-stdenv-passthru).
+
+Existing packages may be modified to pass `__cleanAttrs = "warn";`, so that the legacy attributes remain available, but they will print a helpful warning when they are accessed. Doing this is best avoided until the packaging function used supports recursively defined arguments like [`mkDerivation` does](#mkderivation-recursive-attributes).


Doing this is best avoided until the packaging function used supports recursively defined arguments like mkDerivation does

Which packaging function?

infinisil · 2023-04-03T17:54:56Z

pkgs/test/stdenv/default.nix

+      # optional, but specified in our case
+      "version"


I don't see why version should get propagated here.

infinisil · 2023-04-03T18:00:12Z

pkgs/stdenv/generic/make-derivation.nix

 in

 lib.extendDerivation
  validity.handled
-  ({
+  (
+    lib.optionalAttrs (__cleanAttrs != true) (maybeWarnCleanAttrs {


This removes inputDerivation when __cleanAttrs == true, I'm not sure if that makes sense, since .inputDerivation is meant to be a public stable interface. While it might not be needed anymore since mkShell is now buildable, I'm not sure if this should be decided here.

infinisil · 2023-04-03T18:01:19Z

pkgs/stdenv/generic/make-derivation.nix

+  (
+      (
+        if __cleanAttrs == true
+        then builtins.intersectAttrs { version = null; } drvAttrs


Why is version propagated? I guess it's somewhat important and stable enough. Should definitely be a comment here

infinisil · 2023-04-03T18:06:29Z

pkgs/applications/misc/hello/salve-mundi.nix

+    restore-default-greeting = callPackage ./test.nix {
+      hello = finalAttrs.finalPackage.overrideAttrs (o: {
+        passthru = o.passthru // {
+          greeting = null;
+        };
+      });
+    };


What's this for?

not everyone speaks latin 😄

That is a good point. It could come across as elitist, especially in this context now that I think about it more.
Something like Esperanto would be nicer, or Lojban, or Tolkien's Elvish. Or maybe it should just be hello_with_bells_and_whistles, although I don't know if _ for variations should be followed by snake case or camelCase.
Why is this hard...

May the first confirmation win:

suilad-ambar: is this a correct Elvish package name?

coi-munje: is this a correct Lojban package name?

I don't like the Esperanto version because it's basically Romance language vocabulary. Nothing wrong with that, but if it's not going to be English, I don't want to bias it. Sue me. ;)

fricklerhandwerk · 2023-04-04T23:31:55Z

doc/stdenv/stdenv.chapter.md

@@ -362,6 +364,42 @@ Unless set to `false`, some build systems with good support for parallel buildin

 ### Special variables {#special-variables}

+#### `__cleanAttrs` {#var-__cleanAttrs}
+
+By default, `mkDerivation` will expose its arguments in the returned package attribute set. This is unnecessary and leads to some confusion and doubt.


Suggested change

By default, `mkDerivation` will expose its arguments in the returned package attribute set. This is unnecessary and leads to some confusion and doubt.

By default, `mkDerivation` will expose its arguments in the returned package attribute set.

This is often unnecessary and may lead to confusion about what's relevant for consumers.

We should soften that a bit, because I don't see evidence for the originally definitive-sounding statement.

fricklerhandwerk · 2023-04-04T23:39:27Z

doc/stdenv/stdenv.chapter.md

+
+##### What to do when warned {#warning-package-attr-impl-detail}
+
+If you encounter the warning `The attribute ... of package ... is an implementation detail`, you are invited to help us explicitly support ways in which a package attribute set may used, so that Nixpkgs contributors and users will be aware of your use case.


This could be phrased to be more immediately actionable.

Suggested change

If you encounter the warning `The attribute ... of package ... is an implementation detail`, you are invited to help us explicitly support ways in which a package attribute set may used, so that Nixpkgs contributors and users will be aware of your use case.

If you encounter the warning `The attribute ... of package ... is an implementation detail`, please make Nixpkgs contributors and users aware of your particular use case, and help providing explicit ways to use package attribute sets, by opening an [issue](https://github.com/NixOS/nixpkgs/issues) or (https://github.com/NixOS/nixpkgs/pulls).

nixos-discourse · 2023-05-16T15:04:01Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-16-nixpkgs-architecture-team-meeting-38/28223/1

nixos-discourse · 2023-06-13T20:02:29Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/zurich-23-05-zhf-hackathon-and-workshop-report/29093/1

roberth · 2023-12-01T01:00:22Z

#271241 (comment) shows that access to values computed by mkDerivations can be important for the package definition itself. Ideally this would not go through the public interface, but I'm not sure that I would want to impose finalAttrs.finalPackage.internals.doCheck as a best practice...

Perhaps it'd be better to avoid cleanAttrs and instead move to a module-like situation, where the functions are flattened to a single fixed-point, and the attrsets are unflattened so that what used to be layers between the functions are just groups of attributes.

mk2Derivation({ pythonAttrs, setup, drvAttrs, public }:
  # Don't need to get stuff from four levels deep
  # setup: the generic shell script from "stdenv"
  setup.configureFlags = ... (optional drvAttrs.doCheck "--enable-tests") ...;
  setup.doCheck = true; # like mkDerivation { doCheck } argument
  # No more passthru. It's symmetric now.
  public.tests = callPackage ./tests { mypkg = public; };
)

Is the overlay pattern (with 1 layer of hardcoded merging) good enough for this, or do we need more module system-like features...

DavHau · 2023-12-01T04:14:40Z

How about removing the explicit call to mk2Derivation from the definition and instead introduce a new module __class:

{ pythonAttrs, setup, drvAttrs, public }: {
  __class = "mk2Derivation";
  # Don't need to get stuff from four levels deep
  # setup: the generic shell script from "stdenv"
  setup.configureFlags = ... (optional drvAttrs.doCheck "--enable-tests") ...;
  setup.doCheck = true; # like mkDerivation { doCheck } argument
  # No more passthru. It's symmetric now.
  public.tests = callPackage ./tests { mypkg = public; };
}

That way it would be discoverable how to import this package and it feels like getting one step closer towards better integration with nixos modules.

And of course, automatic merging of values would be nice, but maybe it's not a must for the start.

I think this would already be a huge improvement over the current situation.

roberth · 2023-12-04T12:52:47Z

automatic merging of values would be nice, but maybe it's not a must for the start.

Not merging those "top level" attributes, of which we have only few, will make any sort of overriding incredibly needlessly labor intensive, while gaining very little in flexibility or performance (if at all).
I believe we may allow a restricted form of module system-like merging, as the merging implemented in minimod appears quite cheap, especially if we can get rid of priorities, thanks to the implicit priority encoded by overlay layer ordering.

__class

We already have _class in the module system (newish feature) which is named too similarly, yet must be quite different in interpretation to coexist.

Whether similarity - let alone compatibility - with the module system is a valid requirement remains to be seen, because we can not degrade performance by any significant amount, due to the scale of a typical package's dependency closure.

roberth · 2023-12-12T09:33:15Z

This idea was good, but mkDerivation can not sustain it.
It appeared as low hanging fruit, but it was ever so slightly too much, as exemplified by #217243 (comment).

EDIT: last discussion may resume in #273815

github-actions bot added the 6.topic: stdenv Standard environment label Feb 19, 2023

roberth added 0.kind: enhancement Add something new 6.topic: hygiene labels Feb 19, 2023

roberth force-pushed the clean-packages branch 3 times, most recently from c9f1919 to f4823d6 Compare February 20, 2023 15:32

roberth force-pushed the clean-packages branch from 05f723b to 4002775 Compare February 20, 2023 21:12

Artturin self-requested a review February 20, 2023 21:13

roberth marked this pull request as ready for review February 20, 2023 23:08

roberth requested review from Ericson2314, matthewbauer, edolstra and infinisil as code owners February 20, 2023 23:08

SuperSandro2000 reviewed Feb 24, 2023

View reviewed changes

pkgs/applications/misc/hello/default.nix Outdated Show resolved Hide resolved

mkDerivation: Add __cleanAttrs = "warn";

56a4141

The implementation with dynamic attributes is a little weird, but it performs better by avoiding some attrset copying and sorting, `//`.

roberth force-pushed the clean-packages branch from 78ba831 to 56a4141 Compare March 14, 2023 21:53

SuperSandro2000 reviewed Apr 1, 2023

View reviewed changes

infinisil requested changes Apr 3, 2023

View reviewed changes

fricklerhandwerk reviewed Apr 4, 2023

View reviewed changes

infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023

infinisil mentioned this pull request May 15, 2023

Idea for package creation: using mkOption's apply argument nixpkgs-architecture/pkgs-modules#11

Open

roberth mentioned this pull request Jun 22, 2023

let derivations expose .inputDrvs and .inputSrcs NixOS/nix#5036

Draft

roberth mentioned this pull request Oct 12, 2023

_module.public for standardized module return value? #260778

Open

infinisil mentioned this pull request Nov 19, 2023

Allow custom printing in nix repl NixOS/nix#9380

Open

pbsds mentioned this pull request Nov 30, 2023

treewide: finalAttrs.doCheck -> finalAttrs.finalPackage.doCheck #271241

Merged

13 tasks

roberth marked this pull request as draft December 1, 2023 01:00

This was referenced Dec 5, 2023

Simplify flake.nix by creating generic callPackage-able package.nix NixOS/nix#9535

Merged

Opt-in for running installCheckPhase on cross builds #273110

Closed

roberth closed this Dec 12, 2023

roberth mentioned this pull request Dec 12, 2023

Bring all packaging layers into a single fixpoint #273815

Open

roberth mentioned this pull request Jan 20, 2024

Implicit attribute defaults/overrides inside package definition #273534

Open

infinisil added the 6.topic: architecture Relating to code and API architecture of Nixpkgs label Mar 11, 2024

tie mentioned this pull request Aug 16, 2024

stdenv: fix env pass through with structured attrs #334772

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`__cleanAttrs`: Clean packages that don't expose all internals #217243

`__cleanAttrs`: Clean packages that don't expose all internals #217243

roberth commented Feb 19, 2023 •

edited

Loading

Artturin commented Feb 21, 2023 •

edited

Loading

winterqt commented Feb 21, 2023 •

edited

Loading

roberth commented Feb 21, 2023

SuperSandro2000 commented Feb 24, 2023

roberth commented Mar 16, 2023

SuperSandro2000 commented Apr 1, 2023 •

edited

Loading

SuperSandro2000 Apr 1, 2023

roberth Apr 1, 2023

SuperSandro2000 Apr 1, 2023

SuperSandro2000 Apr 1, 2023

roberth Apr 1, 2023

roberth commented Apr 1, 2023

SuperSandro2000 commented Apr 1, 2023 •

edited

Loading

infinisil left a comment

infinisil Apr 3, 2023

infinisil Apr 3, 2023

infinisil Apr 3, 2023

infinisil Apr 3, 2023

infinisil Apr 3, 2023

blaggacao May 19, 2023

roberth May 19, 2023

fricklerhandwerk Apr 4, 2023

fricklerhandwerk Apr 4, 2023

nixos-discourse commented May 16, 2023

nixos-discourse commented Jun 13, 2023

roberth commented Dec 1, 2023 •

edited

Loading

DavHau commented Dec 1, 2023

roberth commented Dec 4, 2023

roberth commented Dec 12, 2023 •

edited

Loading

		# We allow `hello.src` to be used in tests and examples, despite __cleanAttrs
		passthru.src = finalAttrs.src;


		New packages may pass `__cleanAttrs = true;` to `mkDerivation`, so that it will return a minimal set of package attributes, which package authors can extend via [`passthru`](#var-stdenv-passthru).

		Existing packages may be modified to pass `__cleanAttrs = "warn";`, so that the legacy attributes remain available, but they will print a helpful warning when they are accessed. Doing this is best avoided until the packaging function used supports recursively defined arguments like [`mkDerivation` does](#mkderivation-recursive-attributes).

	By default, `mkDerivation` will expose its arguments in the returned package attribute set. This is unnecessary and leads to some confusion and doubt.
	By default, `mkDerivation` will expose its arguments in the returned package attribute set.
	This is often unnecessary and may lead to confusion about what's relevant for consumers.


		##### What to do when warned {#warning-package-attr-impl-detail}

		If you encounter the warning `The attribute ... of package ... is an implementation detail`, you are invited to help us explicitly support ways in which a package attribute set may used, so that Nixpkgs contributors and users will be aware of your use case.

__cleanAttrs: Clean packages that don't expose all internals #217243

__cleanAttrs: Clean packages that don't expose all internals #217243

Conversation

roberth commented Feb 19, 2023 • edited Loading

Description of changes

Things done

Artturin commented Feb 21, 2023 • edited Loading

winterqt commented Feb 21, 2023 • edited Loading

roberth commented Feb 21, 2023

SuperSandro2000 commented Feb 24, 2023

roberth commented Mar 16, 2023

SuperSandro2000 commented Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Apr 1, 2023

SuperSandro2000 commented Apr 1, 2023 • edited Loading

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented May 16, 2023

nixos-discourse commented Jun 13, 2023

roberth commented Dec 1, 2023 • edited Loading

DavHau commented Dec 1, 2023

roberth commented Dec 4, 2023

roberth commented Dec 12, 2023 • edited Loading

`__cleanAttrs`: Clean packages that don't expose all internals #217243

`__cleanAttrs`: Clean packages that don't expose all internals #217243

roberth commented Feb 19, 2023 •

edited

Loading

Artturin commented Feb 21, 2023 •

edited

Loading

winterqt commented Feb 21, 2023 •

edited

Loading

SuperSandro2000 commented Apr 1, 2023 •

edited

Loading

SuperSandro2000 commented Apr 1, 2023 •

edited

Loading

roberth commented Dec 1, 2023 •

edited

Loading

roberth commented Dec 12, 2023 •

edited

Loading