systemFlake: make one unified outputsBuilder #58
Conversation
| , outputsBuilder ? channels: { | ||
| packages = packagesBuilder channels; | ||
| defaultPackage = defaultPackageBuilder channels; | ||
| apps = appsBuilder channels; | ||
| defaultApp = defaultAppBuilder channels; | ||
| devShell = devShellBuilder channels; | ||
| checks = checksBuilder channels; | ||
| } |
There was a problem hiding this comment.
As you can imagine, this would have a couple consequences.
- We create all the outputs whether users want them or not
- I think
flake checkerrors out from empty attrsets in defaultPackage and defaultApp
There was a problem hiding this comment.
Maybe we can use something like this instead?
deprecatedBuilders = channels: {}
// optionalAttrs (packagesBuilder != null) (packagesBuilder channels)
// optionalAttrs (defaultPackageBuilder != null) (defaultPackageBuilder channels)
// optionalAttrs (appsBuilder != null) (appsBuilder channels)
// optionalAttrs (defaultAppBuilder != null) (defaultAppBuilder channels)
// optionalAttrs (devShellBuilder != null) (devShellBuilder channels)
// optionalAttrs (checksBuilder != null) (checksBuilder channels);
replace output-specific builder
| outputs = mapAttrs | ||
| (outputName: output: otherArguments.${outputName}.${system} or { } // output) | ||
| (outputsBuilder pkgs); |
There was a problem hiding this comment.
I've tried filtering for empty attrsets or null here, but that can cause infinite recursion if any output depends on another.
There was a problem hiding this comment.
Maybe we can be a little more restrictive and require users to set default* in terms of self.packages.${system}.mypackage instead of provide the possibility of a full blown builder, that receives channels attribute. (Same for apps.)
What do you think?
The implication would be that "defaults" specifically depend on "non-defaults", which semantically kind of makes sense anyhow.
There was a problem hiding this comment.
That is generally a good practice. I have no idea how to implement a restriction for that. If we try and check if something is referring to self we would probably run into infinite recursion.
| , defaultAppBuilder ? null | ||
| , devShellBuilder ? null | ||
| , checksBuilder ? null | ||
| , outputsBuilder ? channels: { |
There was a problem hiding this comment.
Naming: I really like outputsBuilder. I don't think we should use exports. Any better ideas?
|
I don't want to ruin your fun but before bed, I thought to play around with it and I think I got a perfectly working version of this. I Will push in just a bit |
Please go ahead, if you want to open a separate PR thats fine too. I got something working, but there still some errors with my version. |
|
Yeah, I opened a separate one #59 I just need to double check CI output, but it should be good |
replace output-specific builders
This is actually much simpler to implement than the old method. And the code looks waay better.
The hardest part is deprecating the current output-specific builders. I have no idea how to do that. I have tried many things like filtering for builders that return null. Either we end up with the flake just creating all the outputs, or infinite recursion when filtering.
implements/closes #56