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

Document system.build.toplevel #155883

Closed

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 20, 2022

Motivation for this change

system.build.toplevel isn't internal anymore, so it should be documented. For example the Flakes article on the unofficial wiki mentions it. It is a very important interface, so I don't think we should try to hide it.

Blocked on #146882

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Extracting system.build into its own module allows other modules
that do not depend on e.g. toplevel to be evaluated, used and
tested in isolation.
This allows the values below it to be specified as options, while
remaining compatible with existing code.
@roberth roberth changed the title Document system build toplevel Document system.build.toplevel Jan 20, 2022
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 20, 2022
@ncfavier
Copy link
Member

Why is this blocked on #146882? Submodule types can be merged IIRC so you should just be able to do

# nixos/modules/system/activation/top-level.nix
{
    system.build = mkOption {
      type = types.submoduleWith {
        modules = [{
          options.toplevel = mkOption { ... };
        }];
      };
    };
}

What do you mean by "system.build.toplevel isn't internal"?

@roberth
Copy link
Member Author

roberth commented Jan 20, 2022

Why is this blocked on #146882?

This PR does not evaluate:

error: The option `system.build' in `nixpkgs/nixos/modules/system/build.nix' is a prefix of options in `nixpkgs/nixos/modules/system/activation/top-level.nix'.

This behavior could be considered an artifact of the module system's development history, but if you stop squinting, it's a bug :)

@ncfavier
Copy link
Member

Sure, but it doesn't block the PR in that we can just comply with the quirky module system for now

@roberth
Copy link
Member Author

roberth commented Jan 20, 2022

Sure, but it doesn't block the PR in that we can just comply with the quirky module system for now

Right, but we'll be creating a mess and setting a bad example of useless complexity. In this case I'd rather wait. Maybe I am using "blocked" too liberally.

@roberth
Copy link
Member Author

roberth commented Jan 25, 2022

#156503 needed some of the same changes, so I've included it there instead. Closing.

@roberth roberth closed this Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants