Skip to content

Conversation

faukah
Copy link

@faukah faukah commented Sep 12, 2025

Changes done:

  • Format using the official nixpkgs formatter instead of the outdated one.
  • Stop shadowing pkgs.
  • Drop flake-utils, instead opting for iterating over systems using pure Nix functions.
  • Fix removing of ocamlformat properly:
nativeBuildInputs = lib.remove pkgs.ocamlformat (testNativeBuildInputs pkgs);

Removes pkgs.ocamlformat from testNativeBuildInputs, while testNativeBuildInputs uses the ocamlformat versio from .ocamlformat. If these ever mismatch, this breaks.

@faukah faukah changed the title flake.nix: cleanup, drop flake-utils, stop shadowing pkgs. flake.nix: cleanup, drop flake-utils, stop shadowing pkgs Sep 12, 2025
@amaanq amaanq force-pushed the fix-nix branch 2 times, most recently from 229ed08 to 99e4c42 Compare September 12, 2025 09:36
@Alizter
Copy link
Collaborator

Alizter commented Sep 12, 2025

Thanks for the PR. Can we split the first two commits off into a separate PR?

@spikespaz
Copy link

spikespaz commented Sep 12, 2025

@Alizter Why does it matter if you avoid a merge commit? They're already separate changes, to show the cause (changing the formatter) and the effect (after formatting).

Edit: Though, they should be reversed.

@Alizter
Copy link
Collaborator

Alizter commented Sep 12, 2025

@spikespaz Its easier for reviewers when PRs are more focused. I asked for the first two commits to be split off because it is uncontroversial and is just a formatting change. The remaining commits will require more attention so it doesn't make sense to tie the first two down. Not sure what you mean about avoiding a merge commit however.

in

pkgs'.mkShell {
pkgs.mkShell {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this matter? I think we really wanted to shadow pkgs here but we can't so we introduced pkgs', however I think its quite confusing to mix using the two.

Copy link
Author

Choose a reason for hiding this comment

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

The mkShell in pkgs and pkgs' are the same, so it makes no difference; I'd argue that this is better though because everyone knows pkgs.mkShell. For pkgs'.mkShell you'd have to look at pkgs' first to be 100% sure mkShell is what you think it is.

It's wise to always keep the scope as minimal as possible for stuff like this.

@Alizter
Copy link
Collaborator

Alizter commented Sep 19, 2025

I can't tell if bumping the flake lockfile caused the issues, but we best do that in a separate PR. There might be unrelated changes that we will have to make and it will cloud this PR.

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.

3 participants