Skip to content

fix darwin build#409

Merged
MattSturgeon merged 2 commits into
NixOS:masterfrom
figsoda:push-otspkmkoquut
May 31, 2026
Merged

fix darwin build#409
MattSturgeon merged 2 commits into
NixOS:masterfrom
figsoda:push-otspkmkoquut

Conversation

@figsoda
Copy link
Copy Markdown
Member

@figsoda figsoda commented May 31, 2026

also enables ci (only the nix-build -A ci part) on darwin, we can test aarch64-linux too but it might be out of scope for this pr

haskell/cabal#4739 (comment)

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 31, 2026

Nixpkgs diff

@figsoda
Copy link
Copy Markdown
Member Author

figsoda commented May 31, 2026

not sure if buildStatic would be useful on darwin, maybe we should remove it

Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

not sure if buildStatic would be useful on darwin, maybe we should remove it

So long as we get enough cache hits that it doesn't impact CI time-to-green I don't feel strongly.

If we ever plan to attach other platforms' static builds to releases, then CI should build them too. If not, I agree it's pointless work.

We could add a build_attrs field to the job matrix if that helps? Or just make the ci attr's content platform-specific.

Comment thread nixfmt.cabal
Comment on lines +41 to +42
-- the executable name nixfmt and the library namespace Nixfmt can collide on
-- darwin due to case insentivity on APFS
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we fix the underlying naming conflict?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

changing to executable: Nixfmt would fix the conflict, but we would need an additional step moving the binary from Nixfmt to nixfmt. I looked through haskell/cabal#4739 and some relevant threads but couldn't find a better alternative

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What a mess. This seems like a fine solution.

Comment thread .github/workflows/main.yml Outdated
Comment thread .github/workflows/main.yml Outdated
@figsoda figsoda force-pushed the push-otspkmkoquut branch from a9cbcea to 91d5c84 Compare May 31, 2026 18:24
@figsoda
Copy link
Copy Markdown
Member Author

figsoda commented May 31, 2026

So long as we get enough cache hits that it doesn't impact CI time-to-green I don't feel strongly.

buildStatic is taking a long time on darwin-build-box due to the lack of existing cache, this might be an issue whenever nixpkgs is bumped

Comment thread .github/workflows/main.yml Outdated
Comment thread .github/workflows/main.yml
Comment thread nixfmt.cabal
Comment on lines +41 to +42
-- the executable name nixfmt and the library namespace Nixfmt can collide on
-- darwin due to case insentivity on APFS
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What a mess. This seems like a fine solution.

@figsoda figsoda force-pushed the push-otspkmkoquut branch from 91d5c84 to e70b5c9 Compare May 31, 2026 18:56
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

So long as we get enough cache hits that it doesn't impact CI time-to-green I don't feel strongly.

buildStatic is taking a long time on darwin-build-box due to the lack of existing cache, this might be an issue whenever nixpkgs is bumped

Let's try it out and play it by ear.

Thanks again!


- name: checks
run: nix-build -A ci
run: nix-build -A ci --argstr system "${{ matrix.system }}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is totally fine here, as matrix.system is a controlled value.

But generally speaking, I try to avoid interpolating GHA expressions into shell scripts (any scripts, tbh).

Since these values don't contain any shell syntax, we could even drop the quotes to avoid the illusion of safety.

The robust general-case solution would be to pass matrix.system in via env.

No changes needed, just commenting to raise awareness.

@MattSturgeon MattSturgeon merged commit d673238 into NixOS:master May 31, 2026
2 checks passed
@figsoda figsoda deleted the push-otspkmkoquut branch May 31, 2026 19:15
@MattSturgeon MattSturgeon mentioned this pull request May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants