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

modules/nixpkgs: initial pkgs option; drop defaultPkgs specialArg #2301

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

MattSturgeon
Copy link
Member

@MattSturgeon MattSturgeon commented Sep 24, 2024

Subset of #2022, working towards #1784

Note for end-users

This PR allows you to configure a pkgs for nixvim's submodule that is different to the one used in your "host" configuration. In this context, a "host" configuration could be home-manager, nixos, or nix-darwin.

For example, this means you don't neccessarily have to use nixvim's standalone build in order to use nixvim's main branch together with a "stable" nixpkgs.

Assuming you have inputs.nixpkgs-unstable in your flake and you pass inputs into your module config somehow (e.g. via specialArgs) you can do:

{ inputs, pkgs, ... }:
{
  # Use nixpkgs-unstable for nixvim:
  programs.nixvim.nixpkgs.pkgs = inputs.nixpkgs-unstable.legacyPackages.${pkgs.stdenv.hostPlatform.system};
}

You could use a let-in block to make the above more readable:

{ inputs, pkgs, ... }:
let
  inherit (pkgs.stdenv.hostPlatform) system;
  pkgs-unstable = inputs.nixpkgs-unstable.legacyPackages.${system};
in
{
  programs.nixvim.nixpkgs.pkgs = pkgs-unstable;
}

Note

In the future we plan to simplify this further, by using the nixpkgs from our flake inputs by default.

This would mean you wouldn't have to do anything in order to use nixvim's main branch with a stable "host" configuration.

However, this would be a breaking change for anyone relying on a specific pkgs instance (configured in their host modules) also being used by their nixvim config.

Overview

I've implemented only the nixpkgs.pkgs option, omitting other nixpkgs.* options that would allow instantiating a nixpkgs instance internally for now for the sake of simplicity.

This is enough to allow end-users to set programs.nixvim.nixpkgs.pkgs to something other than their host module's pkgs, e.g. allowing nixvim unstable to be used on nixos stable without resorting to a standalone nixvim.

  • The nixpkgs.pkgs option is used to allow an "externally constructed" nixpkgs instance to be used as the pkgs arg.
    (implemented)
  • The rest of nixpkgs.* is used to "internally" construct a nixpkgs instance, by evaluating a nixpkgs source path with the configured config, system, overlays, etc.
    (unimplemented)

Internally, this also allows the initial migration of defaultPkgs -> pkgsPath, however I've ran into a couple roadblocks:

  • We still need pkgsPath to be a specialArg because we import using it. Maybe this is a bad idea and we should just copy the upstream modules?
  • If pkgsPath is a "string with context", then the meta module runs into errors. This means we can't just set pkgsPath = inputs.nixvim because that has context.

@MattSturgeon
Copy link
Member Author

MattSturgeon commented Sep 24, 2024

I'm starting to think attempting to pass a pkgsPath in as a specialArg is an anti-pattern. TBH specialArgs are always a last resort for when _module.args or another module option can't be used.

Looking at home-manager's implementation, they add the modules they import from nixos directly into the modules list passed to lib.evalModules. This avoids the need for a specialArg, but is (technically) no less portable.

Nix-darwin simply copy the relevant modules into their repo, avoiding the need to import anything from nixos's modules. They also define a custom option nixpkgs.source which is used as the nixpkgs path to import. This is done using normal option definitions, no specialArgs.

If we want to move those modules into our repo tree, we should do that in a dedicated PR: #2302

@MattSturgeon MattSturgeon marked this pull request as ready for review September 24, 2024 01:04
@MattSturgeon MattSturgeon marked this pull request as draft September 24, 2024 11:26
@MattSturgeon

This comment was marked as resolved.

@MattSturgeon MattSturgeon force-pushed the nixpkgs_module_minimal branch 2 times, most recently from 49fad07 to 7da114b Compare September 24, 2024 14:31
@MattSturgeon
Copy link
Member Author

MattSturgeon commented Sep 24, 2024

I've dropped the pkgsPath stuff from this branch. For now it is enough to simply remove defaultPkgs.

In combination with #2307 and #2302, we will now be able to remove lib.nixvim.modules.specialArgs and specialArgsWith from our public API, as they can be entirely internal to evalNixvim.

I will amend whichever PR is open longer to remove those from our public API.
EDIT: Done in this PR

@MattSturgeon MattSturgeon marked this pull request as ready for review September 24, 2024 14:35
@MattSturgeon MattSturgeon requested a review from a team September 24, 2024 14:35
@MattSturgeon MattSturgeon changed the title modules/nixpkgs: minimal init with pkgsPath modules/nixpkgs: minimal init, dropping defaultPkgs Sep 24, 2024
@MattSturgeon MattSturgeon force-pushed the nixpkgs_module_minimal branch 5 times, most recently from ebf6379 to c0f2898 Compare September 25, 2024 20:38
@MattSturgeon MattSturgeon changed the title modules/nixpkgs: minimal init, dropping defaultPkgs modules/nixpkgs: initial pkgs option; drop defaultPkgs specialArg Sep 25, 2024
lib/modules.nix Outdated Show resolved Hide resolved
wrappers/darwin.nix Outdated Show resolved Hide resolved
lib/tests.nix Outdated Show resolved Hide resolved
@MattSturgeon MattSturgeon force-pushed the nixpkgs_module_minimal branch 2 times, most recently from 0a49f99 to 6abf055 Compare September 27, 2024 02:16
@khaneliman
Copy link
Contributor

khaneliman commented Sep 27, 2024

@MattSturgeon I feel like this has come up numerous times, did we have an issue associated with this or was it always just discussions and matrix? Or was it just the one you created to track it? Sorry, I'm tired and scatter brained posting... lol

@MattSturgeon
Copy link
Member Author

MattSturgeon commented Sep 27, 2024

I feel like this has come up numerous times, did we have an issue associated with this or was it always just discussions and matrix?

The tracking issue is #1784. But yes, mismatched host/nixvim pkgs versions has come up in several issues and discussions. Those are all closed with the advice to use standalone mode for now, IIRC.

This PR doesn't solve the issue,1 but it does provide users with an alternative tool to solve it themselves.

Footnotes

  1. We need to change our default behaviour to fully solve the issue. It'll be a breaking change, but eventually we should have nixvim default to using the nixpkgs from our flake.lock instead of inheriting the host's packages.

@MattSturgeon MattSturgeon force-pushed the nixpkgs_module_minimal branch 2 times, most recently from e3cbb96 to b856d8e Compare September 27, 2024 08:27
Make `pkgs` available to files submodules by passing _all_ module args
through. We already did this for `specialArgs`.
This minimal implementation allows `nixpkgs.pkgs` to be defined, but
does not implement evaluating an instance from a pkgsPath when _not_
defined.

The `defaultPkgs` specialArg is dropped in favour of `nixpkgs.pkgs`
being defined. If it's not defined, an assertion is thrown.

In the future, a nixpkgs source path can be supplied, defaulting to the
flake's `inputs.nixpkgs`. Along with other `nixpkgs.*` options, this
will allow a `pkgs` instance to be evaluated within the module eval.
@MattSturgeon
Copy link
Member Author

@Mergifyio queue

Copy link
Contributor

mergify bot commented Sep 27, 2024

queue

🛑 The pull request has been merged manually

The pull request has been merged manually at 5020e58

@MattSturgeon MattSturgeon merged commit 5020e58 into nix-community:main Sep 27, 2024
2 checks passed
@MattSturgeon MattSturgeon deleted the nixpkgs_module_minimal branch September 27, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants