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

Add option for defining flake-parts modules for downstream flakes. #61

Merged
merged 2 commits into from
Dec 27, 2022

Conversation

shlevy
Copy link
Contributor

@shlevy shlevy commented Oct 16, 2022

No description provided.

@srid
Copy link
Contributor

srid commented Oct 16, 2022

@shlevy
Copy link
Contributor Author

shlevy commented Oct 16, 2022

Fine by me, defer to project maintainers.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Renaming to flakePartsModules seems like the right thing to do, even if I don't like the longer name. That's something that can be solved with a module system feature later:

ChangeLog.md Outdated Show resolved Hide resolved
modules/flakePartsModules.nix Outdated Show resolved Hide resolved
modules/flakePartsModules.nix Outdated Show resolved Hide resolved
ChangeLog.md Outdated Show resolved Hide resolved
@shlevy
Copy link
Contributor Author

shlevy commented Oct 23, 2022

FYI let me know once the code looks good and I'll git rebase --autosquash down the fixup commits, I just like to use them in PRs that have already been reviewed so reviewers know what's changed and what's stayed the same.

@terlar
Copy link

terlar commented Oct 28, 2022

I was testing this and I am having some issues. I am not sure my issues come from this or from some other part of flake-parts.

If I have a flake (flake-parts-module-provider) that uses flake-parts and then expose a flakePartsModule via flake.flakePartsModule.default. Then use this module in another flake that uses flake-parts in an imports via self.inputs.flake-parts-module-provider you will get infinite recursion.

error: infinite recursion encountered

       at /nix/store/ircpgavsl5v9pipq6f23f1dzmlv8k9q0-source/lib.nix:72:32:

           71|       lib.evalModules {
           72|         specialArgs = { inherit self flake-parts-lib; inherit (self) inputs; } // specialArgs;
             |                                ^
           73|         modules = [ ./all-modules.nix module ];

Now this comes with using any self or inputs within imports as I understand it. It just becomes more apparent with this change as it is more likely that someone will use flake-parts to define the module with this and then consume the module with another flake-parts enabled flake.

I found the work-around to pass in the flake containing the flakePartsModules explicitly via specialArgs:

# ...
   outputs = {
    self,
    flake-parts,
    flake-parts-module-provider,
    ...
  }:
    flake-parts.lib.mkFlake {
      inherit self;
      specialArgs = {inherit flake-parts-module-provider;};
    } {
      systems = ["x86_64-linux" "x86_64-darwin" "aarch64-darwin"];
      imports = [./other-file.nix];
    };
}

Then in other-file.nix:

{ self, ... }: {
  imports = [self.inputs.flake-parts-module-provider.flakePartsModules.default];
  # ...
}

I'm quite new to flake-parts so please forgive my ignorance if I have missed some essential information provided in the docs etc.

Edit:
Okay, I found another work-around that perhaps is the more preferred way. That is to use flakePartsModules imports in the flake.nix to avoid having to rely on the module args which causes the infinite recursion.

# ...
   outputs = {
    self,
    flake-parts,
    flake-parts-module-provider,
    ...
  }:
    flake-parts.lib.mkFlake {inherit self;} {
      systems = ["x86_64-linux" "x86_64-darwin" "aarch64-darwin"];
      imports = [
        flake-parts-module-provider.flakePartsModules.default
        ./nix
      ];
    };
}

@terlar
Copy link

terlar commented Oct 28, 2022

Another case that I haven't found a work-around is that if you want to dog-food your own flakePartsModules in the flake providing the flakePartsModules you reach the same kind of error.

The work-around there is import the file directly instead. But would be nice if I could define flakePartsModules.x and then dog-food that same module in the same flake e.g. imports = [self.flakePartsModules.x].

But perhaps that is asking for too much trouble?

@roberth
Copy link
Member

roberth commented Oct 28, 2022

@terlar This is orthogonal to this PR. The same would happen with the current convention, flake.flakeModule.

explicitly via specialArgs:

This adds an unnecessary indirection and it bloats the specialArgs. I'd import the modules in flake.nix itself, where the inputs are simply in scope. If you insist on importing in a separate module, you could use

{ self, ... }: {
  imports = [inputs.flake-parts-module-provider.flakePartsModules.default];
  # ...
}

forgive my ignorance if I have missed some essential information provided in the docs etc.

We need more "guide" docs. Known problem. You've been forgiven ;)

[error when you] dog-food your own flakePartsModules

This fundamentally can't be done from inside the config fixpoint. You could define the module in a separate file or in a let binding to achieve this.

@roberth
Copy link
Member

roberth commented Oct 28, 2022

@shlevy I find .flakePartsModules.default in the imports too unpleasant of a syntax. I liked the idea of improving the data model and make it consistent with the other module attributes, but I don't see a real user experience improvement compared to .flakeModule, especially as I haven't seen the need for a flake with multiple distinct flake modules yet.

@shlevy
Copy link
Contributor Author

shlevy commented Oct 28, 2022

@roberth Let's separate a few different questions:

  1. Should there be documented and checked option definitions for flake-parts users to expose flake-parts functionality to other flake-parts users, rather than relying on convention?
  2. Should we call these flakeModules or flakePartsModules?
  3. Should we support multiple modules exposed from a single flake?
    a. If so, what should be the way to refer to a single module in the common case?

My answer to 1 is definitely yes, there is no benefit to the current way other than no one had done the work, but now I have.

My answer to 2 is leaning slightly toward flakeModule, though for similar reasons I suspect this project should be called flake-modules instead of flake-parts.

My answer to 3 is yes. In part this is out of a desire for commonality with other flake attributes, there is no fundamental reason why a flake should have just one module and it was surprising to me at least to find a singular here where there are plurals elsewhere. But also I can imagine real uses for it, especially in a world with optional/lazily locked inputs, e.g. haskell.nix could have one module for adding cabal.project-based projects and one for stack.yml-based projects, and if I don't import the stack module I shouldn't need to depend on stackage.

My answer to 3a is: Why not leverage the module system, and make it so the singular gets mapped to .default automatically? So if I set flakeModule, that automatically sets flakeModules.default, and if I check flakeModule, I get flakeModules.default automatically?

@roberth
Copy link
Member

roberth commented Oct 28, 2022

Taking one piece of the other thread here.

@shlevy I find .flakePartsModules.default in the imports too unpleasant of a syntax. I liked the idea of improving the data model and make it consistent with the other module attributes, but I don't see a real user experience improvement compared to .flakeModule, especially as I haven't seen the need for a flake with multiple distinct flake modules yet.

ironically enough a policy that you just disagreed with in one particular instance

I dislike the verbosity of the end user syntax of accessing the data model, not necessarily this part of the flakes data model. Does that explain the misunderstanding or did I miss some other irony?

@shlevy
Copy link
Contributor Author

shlevy commented Oct 29, 2022

@roberth I'm confused now. Do you basically agree with my 4 answers in the comment above then?

@shlevy
Copy link
Contributor Author

shlevy commented Oct 29, 2022

@roberth Updated according to my thinking in #61 (comment)

Note that description for the alias doesn't render in the docs because the dev flake is not up-to-date with the new mddoc stuff.

@shlevy
Copy link
Contributor Author

shlevy commented Nov 10, 2022

@roberth can this move forward?

@roberth
Copy link
Member

roberth commented Nov 10, 2022

The change is not bad, but not without risk.

Good

  • adds documentation
  • defining flakeModules. in multiple places (questionable)

Bad

  • this adds a flake output, unconditionally. Users already complain about useless attributes appearing in the repl and nix flake show
    • might be resolved by filtering out the non-transposed top-level attributes in a postprocessing step after the module system, such as flake.flakeModules and flake.nixosConfigurations, but this makes self more strict and therefore more susceptible to infinite recursions

Status quo: users can already define these attributes themselves

Maybe this should be an optional module, to be explicitly imported by flakes that want to expose a flake module?

@shlevy
Copy link
Contributor Author

shlevy commented Nov 10, 2022

@roberth Done

@shlevy
Copy link
Contributor Author

shlevy commented Nov 10, 2022

but this makes self more strict and therefore more susceptible to infinite recursions

flake-parts users can always just access config for this use anyway, right? This is somewhat orthogonal to this PR but would be nice if e.g. packages.x86_64-linux were simply not exported if no packages are defined.

@shlevy
Copy link
Contributor Author

shlevy commented Dec 13, 2022

@roberth Thoughts?

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

One small change and a rebase 👍

flake.nix Outdated Show resolved Hide resolved
@shlevy
Copy link
Contributor Author

shlevy commented Dec 25, 2022

@roberth OK, ready to go!

@roberth
Copy link
Member

roberth commented Dec 27, 2022

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 27, 2022

Build succeeded:

@bors bors bot merged commit 8bfe944 into hercules-ci:main Dec 27, 2022
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.

None yet

4 participants