Skip to content

chore: add native deduplication to flake checks - #330

Merged
heitor-lassarote merged 1 commit into
serokell:masterfrom
weriomat:dedup_flake_check
Sep 2, 2026
Merged

chore: add native deduplication to flake checks#330
heitor-lassarote merged 1 commit into
serokell:masterfrom
weriomat:dedup_flake_check

Conversation

@weriomat

@weriomat weriomat commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

this should fix #318 by deduplicating the flake paths with a Hashset

@Sntx626 Sntx626 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This does not de-duplicate all flake checks, just flake checks from having the same node provided multiple times...

What is the use-case for this?

@weriomat

weriomat commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

Currently, if I deploy multiple nodes from a single repo (i.e. deploy --targets .#a .#b) it should run nix flake check . for each node/ path for the node. Thus, it should be run two times, my attempt is to deduplicate by the path such as it will only get run once regardless of the amount of nodes deployed from that repo.
That is my understanding of the issue described in #318.
Have I missed something?

@Sntx626 Sntx626 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see, in this case I partially misunderstood the purpose of this PR in my previous comment.

I tested this after applying #341 to be able to pass multiple deployment --targets.
It works as described, though it still evaluates the same flake for multiple hosts.

Fixes the problem described.
LGTM

@weriomat

weriomat commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

Hey what exactly do you mean by evaluating? Evaluating the configuration of the particular host, i.e. the system config or doing the nix flake check?

@Sntx626

Sntx626 commented Oct 2, 2025

Copy link
Copy Markdown

I'm getting multiple evaluations of the same flake (. in this case):

$ deploy --targets .#aux .#backup .#mail .#mon .#web
🚀 ℹ️ [deploy] [INFO] Running checks for flake in .
warning: unknown flake output 'deploy'
warning: The check omitted these incompatible systems: aarch64-darwin, aarch64-linux, x86_64-darwin
Use '--all-systems' to check all.
🚀 ℹ️ [deploy] [INFO] Evaluating flake in .
🚀 ℹ️ [deploy] [INFO] Evaluating flake in .
🚀 ℹ️ [deploy] [INFO] Evaluating flake in .
🚀 ℹ️ [deploy] [INFO] Evaluating flake in .
🚀 ℹ️ [deploy] [INFO] Evaluating flake in .
...

I'm referring to the multiple evaluations that should also be able to be de-duplicated, since one evaluation per flake should result in all nodes.

@weriomat

weriomat commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

That is a misconception on your part. For each node/ profile the nixosConfiguration (via. outputs.nixosConfiguration.config.system.build.toplevel`) is evaluated in that step. As deploy-rs does not know anything about this configuration it cannot assume that the configuration is the same across the nodes/ profiles.
In essence, it could be evaluated in a singe step (for all requested host) instead of for each once, but that complicates the design of the program, the speed-up is not huge and is therefore not used.

@weriomat
weriomat force-pushed the dedup_flake_check branch from e86f8d7 to 114acc3 Compare August 4, 2026 21:48
this should fix [serokell#318](serokell#318)
by deduplicating the flake paths with a Hashset
@weriomat
weriomat force-pushed the dedup_flake_check branch from 114acc3 to 7c6af14 Compare August 4, 2026 22:39

@heitor-lassarote heitor-lassarote left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the fix. The code looks good to me. Tests pass and clippy/fmt are quiet. Did some manual testing on my side and it seems to work. Approving.

@heitor-lassarote
heitor-lassarote merged commit c03082a into serokell:master Sep 2, 2026
13 checks passed
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.

Checks are repeated several times

3 participants