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

Update flake.lock, fix errors, get rid of flake-utils #68

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yannham
Copy link
Member

@yannham yannham commented Jan 10, 2025

Issue: #67

Description

This PR started as a simple flake.lock update in order to solve #67. However, it got quickly out of hands. I'm not sure exactly how that's possible, but it seems a lot of flake outputs were actually invalid - I suspect newer versions of Nix now check the flake schema more thoroughly, and I got a ton of error about ill-formed outputs. Typically, non-system dependent outputs such as NixOS configurations and overlays were put in flake-utils' forEachSupportedSystem, which they shouldn't.

While trying to get the minimal diff to make it work again, I found that flake-utils was more of a burden than a help, typically because we have several outputs that don't fit the output.<system>.xxx convention. So, this PR gets rid of flake-utils following the technique described in you don't need flake-utils. This required to refactor part of the flakes, because now non-system dependent outputs need to access computed values.

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

flake.nix Outdated

let
# Matches pkgs.tree-sitter

Choose a reason for hiding this comment

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

Any particular reason we care about tree-sitter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you caught me red-handed copying stuff someone did to get rid of flake-utils from Topiary 🙃 but no, nothing to do with that (and hi 👋 hope you're doing well !)

flake.nix Outdated Show resolved Hide resolved
@yannham yannham marked this pull request as ready for review January 13, 2025 17:39
@yannham
Copy link
Member Author

yannham commented Jan 13, 2025

I'm not sure what to do about the CI error on cargo dist: this recommends to run cargo dist init, but I can find anything installing the cargo dist command (the cargo-dist crates doesn't). Maybe it's outdated? It can also be solved manually, requiring to download the version of the action, but it's a bit dubious: how did we update in the first place, without breaking the CI?

I'm hesitating between doing what the CI wants, or just ignore this altogether.

@yannham yannham requested a review from YorikSar January 13, 2025 17:42
@YorikSar
Copy link
Member

how did we update in the first place, without breaking the CI?

These versions were updated by order PR #66, and CI was indeed failing there. Per cargo-dist docs, it expects that YAML to match exactly what is generated by the tool:

The happy-path of dist has us completely managing release.yml, and since 0.3.0 we will actually consider it an error for there to be any edits or out of date information in release.yml.

I'd recommend updating cargo-dist to the latest version that uses v4 version (since 0.9.0) of these actions and applying whatever changes to release.yaml it requires. Unfortunately Dependabot doesn't support skipping just one file, so you'll have to keep an eye on its PRs and stop changes to this file manually.

@yannham
Copy link
Member Author

yannham commented Jan 16, 2025

Ok, there was a whole lot of confusion - not least because the cargo dist command is now dist alone, so the recommendation from the error message were outdated and that wasn't really clear to me. Anyway, I've updated dist to the latest version. Happy to get a review on the flake re-org part, @YorikSar !

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