Skip to content

Conversation

dtomvan
Copy link

@dtomvan dtomvan commented May 13, 2025

No description provided.

Copy link
Contributor

@Eveeifyeve Eveeifyeve left a comment

Choose a reason for hiding this comment

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

I would say my reviews are helpful suggestion, I can't necessarily approve unless specifically the pkgs.mkShell part is fix for compatibility and/or the flake-utils usage

@dtomvan
Copy link
Author

dtomvan commented Jun 25, 2025

Sorry for taking a while to get back on this. It slipped through my schedule.

@dtomvan
Copy link
Author

dtomvan commented Sep 5, 2025

Anything else that needs to happen before this can be merged?

Copy link
Contributor

@Eveeifyeve Eveeifyeve left a comment

Choose a reason for hiding this comment

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

A few changes, personally I would apply.

" };",
"",
" outputs = inputs @ { self, nixpkgs, flake-utils, ... }: ",
" flake-utils.lib.eachDefaultSystem (system: {",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please spit the systems up and recommend nix-systems to be used here.

Comment on lines 121 to 126
" systems = [",
" \"x86_64-linux\"",
" \"aarch64-linux\"",
" \"aarch64-darwin\"",
" \"x86_64-darwin\"",
" ];",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please recommend nix-systems to be used here.

@dtomvan
Copy link
Author

dtomvan commented Sep 5, 2025

Thanks for the review once again!

Copy link
Contributor

@Eveeifyeve Eveeifyeve left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants