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

flake/generate-files: move most assertions to check derivation #1956

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

MattSturgeon
Copy link
Member

@MattSturgeon MattSturgeon commented Jul 30, 2024

Move the none-ls and efmls assertions out of the generation scripts and into a separate "test" derivation run by nix flake check and buildbot.

I didn't move the rust-analyzer assertions since they seem more related to the generation script itself, rather than the output it produces.

I've also made some other improvements to the generation script, such as using nixfmt directly and passing all derivations in via string context instead of calling nix build multiple times.

I'll probably go over this again in the daytime since I think the test derivation can be simplified further.

@MattSturgeon MattSturgeon requested a review from traxys July 30, 2024 23:04
@MattSturgeon
Copy link
Member Author

MattSturgeon commented Jul 30, 2024

What commit prefix should we be using for the update/gen-files stuff? It seems we've been somewhat inconsistent...

GaetanLepage
GaetanLepage previously approved these changes Jul 31, 2024
Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

It looks good to me at this point.

For the commit prefix, I would go with flake/generate-files

This moves most assertions out of generate-files and into a check
derivation. This should allow the CI to finish, even when there are
issues.

This also properly tests efmls, which was only checked partially before.

rust-analyzer is not covered because the existing assertions relate more
to edge-cases not handled by the generation script than the result it
builds.
Running `nix fmt` is unnecessary in this case.
@MattSturgeon
Copy link
Member Author

@GaetanLepage I've dismissed your approval since I've updated the implementation. Thanks for your early review though!

I've also tested that the update action is able to run on this branch: https://github.com/MattSturgeon/nixvim/actions/runs/10178853612

I've tested the test itself by adding/removing random entries from the none-ls/efmls package declaration files, and everything seems correct:

none-ls: The following are not listed upstream, but are declared in plugins/none-ls/packages.nix:
- foo
- bar

efmls: The following are not declared in efmls-configs-pkgs.nix:
- foobar

efmls: The following are not listed upstream, but are declared in efmls-configs-pkgs.nix:
- foobarbaz
- prettierd
- write-good

Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Still good to me !

@MattSturgeon MattSturgeon changed the title flake/update: move most assertions to check derivation flake/generate-files: move most assertions to check derivation Jul 31, 2024
@MattSturgeon
Copy link
Member Author

@Mergifyio queue

Copy link
Contributor

mergify bot commented Jul 31, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 3789c69

@mergify mergify bot merged commit 3789c69 into nix-community:main Jul 31, 2024
4 checks passed
@MattSturgeon MattSturgeon deleted the update-test branch August 4, 2024 19:56
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