-
Notifications
You must be signed in to change notification settings - Fork 375
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
[ refactor ] Add a nix overlay #3394
Conversation
5db7fdd
to
2e52e65
Compare
Does offering an overlay need to change so much of the existing flake code? It seems to me like the least disruptive change would be to simply pass the already established idris2, idris2Api, etc. to the overlay nix file. In other words, is it really best to make the overlay the center of everything? |
The main reason I opted to move those package derivation calls into the overlay is that it tends to be much easier to expose an overlay to the rest of the flake outputs, rather than vice versa. E.g. the overlay can be conveniently merged with the nixpkgs set, and the package outputs exposed with On the other hand, providing flake outputs to an overlay is trickier to do in a manner that does not make it more difficult for non-flake overlay users.
I'd be curious to see your take on this and would be happy to see a PR against this one with the approach you'd take! It seems a shame to not take advantage of the benefits of the overlay, but perhaps I'm missing some easier way to make the package outputs accessible to the flake in a non-flake-user friendly manner.
Fwiw, in this case the overlay is just calling the packages as we were previously in flake.nix, the main derivations are still in |
It's possible I am missing something here so let me know if my suggestion below misses the point.
If you move the existing diff --git a/flake.nix b/flake.nix
index 3d3e26eef3..95e357494c 100644
--- a/flake.nix
+++ b/flake.nix
@@ -62,12 +62,6 @@
'';
};
stdenv' = with pkgs; if stdenv.isDarwin then overrideSDK stdenv "11.0" else stdenv;
- in {
- checks = import ./nix/test.nix {
- inherit (pkgs) system stdenv runCommand lib;
- inherit nixpkgs flake-utils;
- idris = self;
- };
packages = rec {
support = idris2Support;
idris2 = idris2Pkg;
@@ -76,10 +70,23 @@
} // (import ./nix/text-editor.nix {
inherit pkgs idris-emacs-src idris2Pkg;
});
- inherit buildIdris;
+ in {
+ inherit packages buildIdris;
+ checks = import ./nix/test.nix {
+ inherit (pkgs) system stdenv runCommand lib;
+ inherit nixpkgs flake-utils;
+ idris = self;
+ };
devShells.default = pkgs.mkShell.override { stdenv = stdenv'; } {
packages = idris2Pkg.buildInputs;
};
+ overlays.default = final: prev: {
+ idris2 = final.idris2Packages.idris2;
+ idris2Packages = prev.idris2Packages // {
+ inherit (packages) support idris2 idris2Api;
+ inherit buildIdris;
+ };
+ };
};
in lib.mkOvrOptsFlake
(opts: flake-utils.lib.eachDefaultSystem (per-system opts) // sys-agnostic);
I don't see it as advantageous to use the overlay to build the flake. I find it makes more sense to me to build the flake as its own thing -- it may have some things in common with I think you've cleaned a couple of things up that are worthwhile regardless of the rest and I am not saying I won't accept any PR that does more than the bare minimum number of line changes. It's more the flipping of the script that feels unnecessary. |
I believe the main difference between this approach, and the approach taken in the PR, is that with the PR approach the overlay is accessible to non-flake users (as mentioned above). Feel free to correct me if I'm wrong and there's still some way for a non-flake user to use this!
I certainly have no grand overlay agenda 😅 If we don't mind about the non-flake-user use-case and would prefer to make minimal changes, I'm happy to change this to the diff you've shared above! |
I'm not sure what the most common way to use an overlay hosted in a third party repo is for a non-flake user, but since Idris2 uses flake-compat, the overlay is accessible with or without it being its own file ( |
How many .nix files does it take to change a lightbulb? |
@mattpolzin that's a good point that the overlay should technically still accessible via the flake.nix file itself. I've added a commit that instead declares the overlay in terms of the package outputs. It's a little more verbose than your diff (as overlays are actually system agnostic) but still less changes than the original PR and hopefully easier to review. |
@@ -0,0 +1,11 @@ | |||
{ buildIdris, idris2Version }: | |||
buildIdris { | |||
src = ./..; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a request for change, I just would not have ever thought to do it this way (as opposed to ../.
) which I thought was interesting. Interesting that such an obviously equivalent path never crossed my mind.
This PR adds a Nix overlay and exposes it from the flake. For context, overlays provide a convenient way to merge a set of packages into a larger nixpkgs set.
The overlay attempts to follow the precedent set by the existing idris2 setup in nixpkgs - it exposes the idris2 packages via
idris2Packages
, then re-exportsidris2
at the top-level. Specifically:pkgs.idris2Packages.buildIdris
pkgs.idris2Packages.idris2
pkgs.idris2Packages.idris2Api
pkgs.idris2Packages.support
pkgs.idris2
To avoid duplication, the package derivation calls have been moved to the overlay and the package outputs changed to make use of the merged
pkgs
set by using the overlay viaself
. Theidris2Api
derivation has been moved into its own file as a part of the refactor.