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

Minor cleanup of flake.nix #3110

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Rexcrazy804
Copy link

@Rexcrazy804 Rexcrazy804 commented Apr 15, 2024

used with pkgs; to simplify statements referring to it in the scope of mkshell. Restructured input declaration to follow the typical structure.

I am unsure whether I should mark this with doc: or not, sorry

@Rexcrazy804 Rexcrazy804 requested a review from teto as a code owner April 15, 2024 16:56
@Rexcrazy804
Copy link
Author

If permitted, I'd like to add a shell hook that executes the default shell using the $SHELL env var so running nix develop will drop you into whatever shell you are using instead of defaulting to bash. I suppose that could be another pr?

@justinmk
Copy link
Member

@teto

flake.nix Outdated Show resolved Hide resolved
@itslychee
Copy link

If permitted, I'd like to add a shell hook that executes the default shell using the $SHELL env var so running nix develop will drop you into whatever shell you are using instead of defaulting to bash. I suppose that could be another pr?

I disagree with this, perhaps check out direnv if you haven't already?

@Rexcrazy804
Copy link
Author

@itslychee I have specified the three major systems, do you reckon there's anything else to add?

@itslychee
Copy link

itslychee commented Apr 17, 2024

@itslychee I have specified the three major systems, do you reckon there's anything else to add?

Don't forget to change buildInputs to packages, and after that I'd squash all the commits into one commit to keep the history clean.

@itslychee
Copy link

Also since eachDefaultSystem is used currently, it's best to add "x86_64-linux" to the platform list to keep it backwards compatible as eachDefaultSystem used 4 platforms as described here

@Rexcrazy804
Copy link
Author

Rexcrazy804 commented Apr 17, 2024

Could you clarify that a bit further? The platforms list does contain "x86_64-linux" but it has come to my attention that "x86_64-darwin" is not included in the list, I suppose that is the one I should add.

@itslychee
Copy link

Could you clarify that a bit further? The platforms list does contain "x86_64-linux" but it has come to my attention that "x86_64-darwin" is not included in the list, I suppose that is the one I should add.

Oops yeah that's the one, something possessed me to write linux instead 😭

] (system: function nixpkgs.legacyPackages.${system});
in {
devShells = forAllSystems (pkgs: {
default = with pkgs; mkShell {
Copy link
Member

Choose a reason for hiding this comment

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

why remove the pkgs prefix ? it's just churn . If anything it makes perf and linter work harder please revert

@teto
Copy link
Member

teto commented May 1, 2024

I get that one would prefer to remove the dependency to flake-utils but what's the point if you have to replace allDefaultSystems by actually typing the defaults yourself ? The handling of system is one of the controversial point of flakes and nothing in this PR is objectively better so it's just churn. I would just merge an update to the lockfile to get a more recent version of the dependencies.

@itslychee
Copy link

what's the point if you have to replace allDefaultSystems by actually typing the defaults yourself

The point here is to remove a dependency that can be easily replaced with functions of nixpkgs' library, and to remove noise from the lockfile. It also makes it more clear how outputs are defined instead of having flake-utils magically insert system between a nested attrset that may not even be desirable.

The handling of system is one of the controversial point of flakes and nothing in this PR is objectively better so it's just churn.

How is eachDefaultSystem not churn then? How is removing an input that's not even needed and replacing the functionality with a genAttrs wrapper while retaining the same behavior with the previous solution not a better approach, as it also gets rid of the rec keyword that's not even used anywhere?

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