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

pkgs/by-name: document toplevel argument restrictions #325262

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jul 7, 2024

We've been bitten by this at least twice, let's make sure to write it down.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

We've been bitten by this at least twice, let's make sure to write it
down.
@flokli flokli force-pushed the docs-pkgs-by-name-toplevel-arg-restrictions branch from 73aaee9 to 7d3eb17 Compare July 7, 2024 13:28
well as those added through the use of overlays will cause defaults to be
overwritten in a unexpected and hard-to-debug fashion.

In case you're intending to write more flexible package expressions, consider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not by-name's mistake. This is about callPackage. Whenever people use callPackage they are affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it's not by-name's mistake.

This also say it's because it's callPackage'd. I still think it's worth to write it down like this, because people tend to be too fancy with what they do in by-name, and restricting it only to packages defined in the top-level (without defaults) and to only boolean enable flags prevents this effectively.

Copy link
Contributor

@eclairevoyant eclairevoyant Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main restriction with by-name is that you can't use a custom callPackage, there's nothing else that I know of affecting the override interface. Besides, there is an upcoming check to enforce moving to by-name when touching packages at the top-level, so this suggestion doesn't make sense.

Let's just drop the sentence and make the guidelines more explicit if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please propose exact wording in a new PR.

@@ -76,6 +76,17 @@ and override its value in [`pkgs/top-level/all-packages.nix`](../top-level/all-p
}
```

### Toplevel argument restrictions
The list of toplevel arguments should be restricted to packages defined in the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, technically nothing hinders someone of creating a package with the exact same name of a Boolean flag.

What about with-shell?
https://github.com/NixOS/nixpkgs/blob/nixos-24.05/pkgs/applications/misc/with-shell/default.nix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have any "non-package args", so don't understand the question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am saying that with-shell is an existing package, and because of callPackage no one can use with-shell as a simple Boolean flag.

@@ -76,6 +76,17 @@ and override its value in [`pkgs/top-level/all-packages.nix`](../top-level/all-p
}
```

### Toplevel argument restrictions
The list of toplevel arguments should be restricted to packages defined in the
common nixpkgs package set, and simple `enable*` boolean flags.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attributes of type string are employed to indicate options. Emacs expression accepts toolkit as "gtk", "motif", "athena"...

And Emacs is outside by-name.

@flokli flokli closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants