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

Cleanup nix code #4265

Merged
merged 9 commits into from
Oct 21, 2024
Merged

Conversation

scottbot95
Copy link
Contributor

Separate out the various pieces of our nix code into their own flake-modules.

I also moved the dev dependencies to their own partition which should substantially speed up evaluation of our flake in consumers since the primary flake.lock is now tiny compared to what it used to be (most notably there is now only 1 version of nixpkgs).

Copy link

netlify bot commented Oct 14, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit a987ede
🔍 Latest deploy log https://app.netlify.com/sites/teslamate/deploys/67163b0828431a0007753865
😎 Deploy Preview https://deploy-preview-4265--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@scottbot95
Copy link
Contributor Author

Looks like the failing checks are all due to failures when trying to push to the main teslamate image repo? Seems like probably a bug in the workflow logic to me.

@brianmay
Copy link
Collaborator

Seems like probably a bug in the workflow logic to me.

You are probably correct. I think the other pull requests were made from this repo.

We have:

REGISTRY_IMAGE: ghcr.io/${{ github.repository }}

I would have assumed that for pull requests this would expand to the source repository. Which I think should be fine.

But it is acting like it has been expanded to the destination repository.

The official documentation is vague and doesn't address this. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#github-context

@scottbot95
Copy link
Contributor Author

Hmmm from this post it sounds like we could probably use github.event.pull_request.head.repo.full_name instead. Although we probably don't want to try to push an image at all if it's a fork, right?

@brianmay
Copy link
Collaborator

Yes, that sounds right. Wish this was better documented.

I think people might want to test out pull requests, even if they do come from a fork...

@scottbot95
Copy link
Contributor Author

Ok I filed #4266 for this and will just ignore those failures in this PR unless we'd rather fix that in this PR.

@JakobLichterfeld
Copy link
Collaborator

I think people might want to test out pull requests, even if they do come from a fork...

This is still possible. ghcr_build runs as pull_request target fine. Only issue is called via workflow_call from a fork, which I will solve.

@JakobLichterfeld JakobLichterfeld added the enhancement New feature or request label Oct 15, 2024
@JakobLichterfeld
Copy link
Collaborator

which I will solve.

solved

@JakobLichterfeld JakobLichterfeld marked this pull request as draft October 19, 2024 12:53
Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld 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 your refactoring!

I tested the formatting successful.

  • Can you find a way not to duplicate elixir version and beamPackages version?
  • nix flake check --override-input devenv-root "file+file://"<(printf %s "$PWD") stopped working on aarch64-darwin system
  • devenv up is not working anymore

from root:

error: flake 'git+file:///Users/jakob/development/repos/teslamate-scottbot95' does not provide attribute 'packages.aarch64-darwin.devenv-up', 'legacyPackages.aarch64-darwin.devenv-up' or 'devenv-up'

from /nix/dev:

error: flake 'git+file:///Users/jakob/development/repos/teslamate-scottbot95?dir=nix/dev' does not provide attribute 'packages.aarch64-darwin.devenv-up', 'legacyPackages.aarch64-darwin.devenv-up' or 'devenv-up'

@scottbot95
Copy link
Contributor Author

As far as de-duplicating the exlir/beamPackages: this might be tricky using flake partitions since the dev partition is fundamentally a separate flake (most notably there is technically a different version of nixpkgs used in the main flake and the dev partition). I think I have some ideas for unifying the elixir/beamPacakges though (or at least ensuring we select the same erlang version from potentially different nixpkgs).

And for the devenv stuff not working anymore: I thought I tested that locally, although I don't have a darwin system (that shouldn't matter though since I didn't touch anything platform specific). I will try to figure out what's going on here.

@scottbot95
Copy link
Contributor Author

@JakobLichterfeld What's the error you get when you try to run the nix flake check command? Is it something like the following? If so I think I know the issue.

error: The option `perSystem.x86_64-darwin.checks.default' was accessed but has no value defined. Try setting the option.

Also I figured out the devenv-up issue, the package is being created, but only inside the dev partition so I just need to plumb it out to the root flake (although for some reason it keeps hanging on evaluation for me). However, I'm actually unsure there is even a point in doing so. Either way, nix is required in order to load the dev environment so why not just unify around using nix develop instead?

@JakobLichterfeld
Copy link
Collaborator

at least ensuring we select the same erlang version from potentially different nixpkgs

Yeah that would be awesome. I do not like to have to maintain one more location if we update Elixir OTP version.

What's the error you get when you try to run the nix flake check command? Is it something like the following? If so I think I know the issue.

Exactly, I do get

❯ nix flake check --override-input devenv-root "file+file://"<(printf %s "$PWD")      
warning: not writing modified lock file of flake 'git+file:///Users/jakob/development/repos/teslamate-scottbot95':
• Updated input 'devenv-root':
    'file:///dev/null?narHash=sha256-d6xi4mKdjkX2JFicDIv5niSzpyI0m/Hnm8GGAIU04kY%3D''file:///dev/fd/11?narHash=sha256-Ffxb7keRyQDuwTUTfYgSgon5ylie6q0FPBccJ%2BmTOjo%3D'
error:
       … while checking flake output 'checks'

         at «none»:0: (source not available)

       … while checking the derivation 'checks.aarch64-darwin.default'

         at «none»:0: (source not available)

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: The option `perSystem.aarch64-darwin.checks.default' was accessed but has no value defined. Try setting the option.

However, I'm actually unsure there is even a point in doing so. Either way, nix is required in order to load the dev environment so why not just unify around using nix develop instead?

I personally prefer pure nix develop, but this was introduced by Brian. @brianmay May you elaborate a bit on devenv up?

@brianmay
Copy link
Collaborator

The alternative is something like mkShell, which has no support for starting or initializing the dependencies (database, mqtt server).

Possibly we could create our own wrapper around process-compose to do that, no idea if this makes any sense. I see there is a nix-flakes module though: https://github.com/Platonic-Systems/process-compose-flake

I suspect we may have to recreate stuff that is already done in devenv though.

@brianmay
Copy link
Collaborator

I personally prefer pure nix develop, but this was introduced by Brian. @brianmay May you elaborate a bit on devenv up?

Might be some confusion here. You still need to use "nix develop" (direnv does that by default), and this provides the "devenv up" command which starts the mqtt, database processes.

@JakobLichterfeld
Copy link
Collaborator

The alternative is something like mkShell, which has no support for starting or initializing the dependencies (database, mqtt server).

Thanks for reminder, was wondering why you choose devenv and stumbled over the process and services which are very neat

@scottbot95
Copy link
Contributor Author

"devenv up" command which starts the mqtt, database processes.

Ahh so devenv up is still needed to start the other services. I think I was confusing it with direnv which doesn't really do anything beyond what nix develop does. Ok I will spend some time tomorrow trying to get devenv working again. I might have to get rid of the partition support, but that's really not a big deal (and also a relatively new feature of flake-parts so unsurprising that there may be rough edges)

@brianmay
Copy link
Collaborator

I am always getting devenv and direnv confused myself, the names are too similar IMHO.

@scottbot95
Copy link
Contributor Author

Well I spent a couple hours poking around at it and couldn't figure out how to get devenv-up working again with partitions. The issue is that since the devenv is created in the dev partition, the devenv-up package is also only in the dev partition. And for reasons I do not understand, trying to lift that package from the dev partition out to the root partition will just cause nix to hang forever when evaluating the package 🤷

So I will just rip out partitions and not worry about it. It would be a slightly nice optimization as it speeds up evaluation speeds of the flake a fair bit (when just building the package), but doesn't actually do anything other than that.

Looks like `mkIf` doesn't work properly with flake-parts/perSystem
@JakobLichterfeld JakobLichterfeld marked this pull request as ready for review October 21, 2024 11:46
Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld 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 your refactoring and additional efforts!

Thanks for finding a nice solution to avoid the duplicate elixir version and beamPackages version?

devenv up is now running as expected.

I resolved the linter issue.

lgtm

@JakobLichterfeld JakobLichterfeld merged commit dffdea9 into teslamate-org:master Oct 21, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants