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

Re-enable basic CI #239

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

jaen
Copy link
Contributor

@jaen jaen commented Jun 7, 2024

This re-enables basic CI checks:

a) treefmt,
b) pytest,
c) nix flake check,
d) nix flake check for the flake generated from the flake template.

It would probably be useful to fix and re-enable more checks, but I think it's better to have those basic ones in earlier and do others as a follow-up.

I have also took the opportunity to add direnv support for convenience and moved flake outputs to a separate file in ./flake so it's a bit cleaner. Let me know if you (don't) mind.

Based on the top of #237, so it needs to go first.

@jaen
Copy link
Contributor Author

jaen commented Jun 7, 2024

This will probably require some changes in the repo settings to enable CI and have a Cachix token, but I do not have access to that, so you'll have to do it after it's merged.

@jaen
Copy link
Contributor Author

jaen commented Jun 7, 2024

Also, possibly outstanding questions:

a) I wanted to add formatting for pipeline YAMLs, but apparently treefmt keeps ignoring dotfiles (numtide/treefmt#153) so not sure if it's worth it. Thoughts?
b) maybe it would be useful to mention something about treefmt/direnv/CI changes in the readme – but there's no place there. I later noticed there's https://github.com/nix-community/robotnix/blob/master/docs/src/development.md I could use, but maybe it would make sense to link to that from README.md or add CONTRIBUTING.md that links to it. What do you think?
c) for checks to actually help we would have to make them required for the branch, I assume we want that?
d) on a semi–related note, do you think it makes sense to consider forcing linear history and/or a merge queue of changes that seem correct on a branch end up not being correct after merge?

@Atemu
Copy link
Contributor

Atemu commented Jun 7, 2024

a) Ugh, what an arbitrary l limitation. That and the fact that it's so tightly coupled to flakes makes me want a different tool. How hard can it be to run a formatter on a bunch of files? fd -g '*.nix' -x nixfmt?
b) Yes, all the dev docs please. I'm not married to any particular location for them.
c) For things that can be automated, yes please.
d) I have yet to see any good reason for linear history and know of lots of reasons against it.

@Atemu
Copy link
Contributor

Atemu commented Jun 7, 2024

What would we need cachix for? We won't be building anything in GH actions.

@jaen
Copy link
Contributor Author

jaen commented Jun 7, 2024

Ugh, what an arbitrary l limitation. (...) makes me want a different tool. How hard can it be to run a formatter on a bunch of files?

Fair, it's an annoying limitation (didn't know if it beforehand, sorry) and it's not very hard to just run formatters — but having a consistent interface for multiple formatters that runs them in parallel and properly reports errors is a wee bit harder, which is why I decided to go with a pre-existing solution. Of course it turns out it's not perfect xD I'd personally be willing to overlook pipeline formatting until they fix that but it seems they are somewhat slacking on maintenance (a version with the --hidden flag that can be used as a workaround didn't get released for 3/4 of a year…) and it's not my personal project for me to call the shots — so if you feel strongly about it, we can keep to a simple wrapper script.

the fact that it's so tightly coupled to flakes

Is it, though? I was under impression it can be used just the same from classic Nix (at least evalModule looks to be exposed just the same in default.nix). It's just that robotnix already had a flake and classic Nix stuff was provided via flake-compat, so I continued in that direction. To be fair, I'm only using Nix for ~4 years and flakes were already the suggested way to do things back then, so I didn't have a chance to grow a preference for classic Nix and don't hate flakes too much (yes, I also experienced their various suckages, but where they pained me the most — heavy, nix-hostile monorepos — doesn't really apply to robotnix, so I didn't assume they would be a problem).

Yes, all the dev docs please. I'm not married to any particular location for them.

Sure, will add some prose to this and #237.

For things that can be automated, yes please.

Sorry if I wasn't clear here — this is something that would have to be toggled in the repository Settings tab (c.f. GitHub docs). I do not have access to that, so someone else will have to do that after this PR is merged. I'm just pointing out that this PR won't help a lot until it will be enforced by setting required checks up.

I have yet to see any good reason for linear history and know of lots of reasons against it.

YMMV I guess, because I'm the reverse — but maybe that's because I'm a shit developer xD More seriously though, bad word choice — linear history is tangential to what I wanted to say. What I did really mean is having branch up to date when running tests — either via GH forcing people to update their branches before merging (which I conflated with linear history, because I usually update via rebase) or having a merge queue.

Because without ensuring the branch is up to date you're liable to end up with a situation where tests pass on branches, but after they are merged together into the master branch, it starts failing due tologically conflicting changes. While maybe there could be some argument made to living with that for the sake of development velocity (which I don't really agree with, but that's just differing priorities, I guess), I'm not sure if it applies to robotnix where maintenance time is very limited, so the master branch could be failing for a long time before someone has time to fix it and then review it. Which is why I raised the issue, because I think setting up something that stops it from happening would be beneficial.

What would we need cachix for? We won't be building anything in GH actions.

Not in this PR, no. But my impression was that there were thing being built in CI before (https://github.com/nix-community/robotnix/blob/master/release.nix) and I'd assume we would eventually do it again when things are building consistently again (and given it would, like the above, require someone else to muck around in Settings, I figured it'd be better to set up both things at once). Having kernels and browsers in a cache is certainly better than having to build them locally and I kind of assumed that if robotnix had done so previously, it's not problematic to do it again. If for some reason it's a problem to do it again, then I guess it could go. But at this point I'm not entirely sure what the benefits of upstreaming things would be, if everyone will have to build and check nothing is broken on their own machines anyway.

@jaen
Copy link
Contributor Author

jaen commented Jun 8, 2024

Re: alternative tooling - I could try and see how https://github.com/cachix/git-hooks.nix works can be used for this, just don't enable githooks in the default shell, if you don't want that to be the default. Could that be an option?

@jaen jaen marked this pull request as draft June 8, 2024 06:47
@jaen
Copy link
Contributor Author

jaen commented Jun 15, 2024

Formatter problems should be resolved now: #237 (comment)

@jaen jaen force-pushed the fix-github-checks-action branch from 8f8c72e to 2bb46e2 Compare June 17, 2024 18:40
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