-
Notifications
You must be signed in to change notification settings - Fork 481
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 Nix Code (remove iogx) #6795
Conversation
nixpkgs-2405.follows = "haskell-nix/nixpkgs-2405"; | ||
|
||
nixpkgs.follows = "haskell-nix/nixpkgs"; |
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.
Used to follow nixpkgs-unstable
, switched to nixpkgs
now. Why?
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.
If I recall correctly, we used to follow nixpkgs-unstable
in order to access some packages not available in nixpkgs
.
But in general we want to follow a stable nixpkgs
if possible.
@@ -2,18 +2,10 @@ | |||
description = "Plutus Core"; | |||
|
|||
inputs = { | |||
|
|||
iogx = { |
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.
By the way, this PR has no explanation, and I am not clear on the following:
- what is the problem with using
iogx
? - why is it used in the first place? What value does it add?
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.
Good point. I added a comment, and a follow up here:
I wanted to explore what the code would look like without iogx and whether the shell would be lighter as a result (it is).
At this point, iogx doesn’t seem to provide enough value to justify its (future) maintenance cost. Removing it also makes the codebase more accessible to future maintainers.
outputs = inputs: inputs.flake-utils.lib.eachDefaultSystem (system: | ||
import ./nix/outputs.nix { inherit inputs system; } | ||
); |
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.
What I understand here is that instead of using iogx
to create flake outputs we're creating outputs ourselves.
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.
Yes iogx worked some magic and included some outputs in addition to the one explicitly defined by the user.
}; | ||
|
||
flake-utils.url = "github:numtide/flake-utils"; | ||
|
||
pre-commit-hooks.url = "github:cachix/git-hooks.nix"; |
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.
IIUC, before this output was added by iogx.lib.mkFlake
, right? Are there any other outputs that were added by iogx
and are not present now?
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.
Yes this input was added by iogx. There are no other hidden inputs.
@@ -1,6 +1,6 @@ | |||
{ repoRoot, inputs, pkgs, system, lib }: |
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.
I don't get what exactly do these changes in the agda-related file have to do with iogx
removal?
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.
iogx expected each nix file to be a function from { repoRoot, inputs, pkgs, system, lib }:
.
This is no longer the case.
@@ -1,12 +1,14 @@ | |||
{ repoRoot, inputs, pkgs, system, lib }: |
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.
Same here: not clear how these changes are related to the iogx
removal.
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.
Same as above
in | ||
|
||
{ | ||
inherit __internal; |
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.
Why is this __internal
indirection needed? Can't you inherit what's int the __internal
directly?
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.
I don't want to pollute the top-level flake namespace with a bunch of things.
This is only used for debugging.
The top-level should only have packages
, devShells
, etc...
|
||
flattened-ci-jobs = utils.flattenDerivationTree ":" nested-ci-jobs; | ||
|
||
ciJobs = utils.flattenDerivationTree ":" nested-ci-jobs.${system}; |
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.
Can you refer to the flattened-ci-jobs
above?
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.
I wish, but no, we'll have to live with a little repetition
@@ -1,6 +1,5 @@ | |||
# editorconfig-checker-disable-file | |||
|
|||
{ repoRoot, inputs, pkgs, system, lib }: | |||
{ inputs, pkgs, lib, agda-with-stdlib, r-with-packages }: |
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.
YaY! this way the dependencies are more explicit, liking it!
@@ -12,18 +11,9 @@ let | |||
|
|||
src = ../.; | |||
|
|||
shell = { |
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.
Why is this shell getting removed?
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.
It has been moved to nix/shell.nix:project.shellFor
which is now the only source of truth for all shell things
@@ -1,4 +1,4 @@ | |||
{ repoRoot, inputs, pkgs, system, lib }: | |||
{ pkgs }: |
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.
its only the rPackages
that's used, I suggest declaring it as a parameter directly, avoiding passing in all the unnecessary pkgs.
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.
I'd rather draw the line at pkgs
and let consumers pull from it.
Also it's both rPackages
and rWrapper
@@ -0,0 +1,16 @@ | |||
{ lib }: | |||
{ | |||
flattenDerivationTree = separator: set: |
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.
This function is only used in the outputs.nix
, I'd simply keep it there.
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.
True but I prefer it here. One day it could be used elsewhere, or nix/utils.nix could have more functions.
iogx was created as a top-level abstraction layer for Haskell projects at IOG. It wraps haskell.nix, simplifying the process of setting up a working Flake. It makes several assumptions and provides additional features like a more powerful development shell, Read the Docs integration, and pre-configured pre-commit hooks.
Since its integration into Plutus, some of these features have become obsolete (e.g., Read the Docs), and we’ve realized we can rely more directly on haskell.nix for the rest.