feat(nix): add comprehensive Nix flake #4826
Conversation
doprz
changed the title
|
I know next to nothing about Nix, and in particular I don't know what Nix flakes are. I don't really want to spend time on learning more about that, either. However, I would have to understand things a little better in order to judge if the PR is good and can be merged. So what can we do? I see that our current README.md already says something about flakes (here), but I don't understand what it means. Would this have to be changed with your PR? Any people reading here who are more familiar with Nix and can help review this? |
|
It seems that someone has already uploaded I will update the README.md to include information about the local nix flake. In summary this will allow people that use nix to run your project in an ephemeral shell, install it, and even have a fully setup dev env with go and other dependencies managed by nix. This will be an official flake that is fully featured while the one on nixpkgs is maintained by others and will either follow the 20.05 or unstable channel. |
|
@Eveeifyeve thank you for the feedback! |
|
I really wonder if this thing should be maintained in our repo at all. It sounds similar to an rpm spec file, a deb file, or a homebrew formula, and we don't maintain those here either; we let people who are packaging experts do it wherever it makes sense to them. I'd feel better about this if we took the same approach for this flake thing too. (Happy to link to it from the readme though, of course.) Or am I missing something about why it conceptionally makes sense to maintain this here, unlike those other ones? |
|
The main difference is that this will let developers load up a dev environment provided by nix which includes all the necessary dependencies, env vars, and build tools/checks setup. I've also added the ability to build from source using nix while the unofficial one on nixpkgs is currently pinned at v0.53.0. The nix configuration doesn't require constant maintenance and will only be updated when NixOS reaches a next major version as the packages follow that or |
|
Thank you for your feedback @Eveeifyeve on this PR! It's been very helpful to learn best practices. |
|
Thank you @Eveeifyeve! |
|
@stefanhaller bump |
stefanhaller
force-pushed
the
feature/nix-flake
branch
from
7dc0e36 to
e357f68
Compare
There was a problem hiding this comment.
A few things.
- The branch is totally unreviewable with all the merges from master, and all the fine-grained commits that change things back and forth. I rebased onto master to get rid of the merge commits and force-pushed, this makes it a little bit reviewable at least. In the end I suppose all commits need to be squashed into one. We don't do this by default here, it's the contributor's job to provide a commit sequence that makes sense. In the future, please look into working with
fixup!commits, and never merge master into your branch; always rebase onto master instead. - As for maintaining it here vs. elsewhere: your explanation above still didn't make it very clear to me what the benefit is of maintaining the flake in this repo vs. somewhere else. I now saw that we have an issue for this (#3474), this explains it a little bit better. But still, there are also things like flakehub, would it be an option to use that instead?
The reason why I'm still hesitant to host the flake here is that this PR has shown that it needs expertise to get this right. @Eveeifyeve seems to have done a great job at helping with this, but if they are not around next time we need to make a change to this, who else will be able to do that? I won't, because I totally lack the expertise, and I don't intend to acquire it. @Eveeifyeve What's your take on this?
flake.nix
Outdated
|
|
||
| buildInputs = with pkgs; [ | ||
| # Go toolchain | ||
| go_1_24 |
There was a problem hiding this comment.
I don't know the nix language at all, so not sure this makes sense, but is there some way to not hard-code the version here, but extract it from go.mod somehow? It would be nice to have fewer places that need to be touched when we bump our go minimum version; I'm worried that we will forget to update it here.
There was a problem hiding this comment.
I would say base it on the latest go version.
Here is a suggestion to fix this:
| go_1_24 | |
| go |
There was a problem hiding this comment.
I don't think this is appropriate for guaranteeing a stable environment, which I understand is the whole point here.
While go promises not to break the build with language updates, it might still break tests. It looks like this happened in #4844, so currently developing lazygit works with 1.24 but not with 1.25. (I actually haven't investigated those test failures yet, so I don't even know if these only affect running the tests or also introduce regressions in the production code.)
There was a problem hiding this comment.
Well it's impossible of getting the version from the go mod unless you do a script with sed and some magic like make it a package in list which requires huge amounts of nix code to make it work and out of my reach.
I would have to research how packages with the list in pkgs.mkShell work and create a function for it to work.
Many go projects add a defaulted go version in a nix flake to use and call it a day.
If you want to try go right ahead, but for now we should stick to the latest go package since we can't read the go mod file.
There was a problem hiding this comment.
@doprz Please don't mark things as resolved when they aren't.
@Eveeifyeve In that case we should go back to specifying the version explicitly and accept the redundancy. We do the same in the Dockerfile and in .github/workflows/ci.yml and other places. It is important to be on the same version everywhere to guarantee predictable results.
The problem is that those other places can easily be found by a global search, but the one in the flake can't because it uses a different format. To mitigate this, we should probably add a comment such as:
diff --git i/go.mod w/go.mod
index 324be6d7e..f48ce401b 100644
--- i/go.mod
+++ w/go.mod
@@ -1,5 +1,7 @@
module github.com/jesseduffield/lazygit
+// If you change this, be sure to also update flake.nix and other files that
+// mention the go version (search for it)
go 1.24.0
require (There was a problem hiding this comment.
@Eveeifyeve Good point, I guess the regex could be a bit stricter, maybe something like "^go\\s+(\\d+\\.\\d+)\\.\\d+$". We don't need to be lenient against the version having either two or three components; we know it will always be three.
@doprz Did you actually test your version? I think you need to double the \ in the pattern to escape it properly.
Can we add some error checking to make sure the whole thing errors out when it doesn't find a match?
Finally, I'd consider matching the first two components of the version separately so that you don't need to replace dots by underscores afterwards.
There was a problem hiding this comment.
Yes, I'm able to add some error correction. Note that in nixpkgs, go has the following options hence why I'm only matching the first two parts of the version:
There was a problem hiding this comment.
Yeah that could work, but only problem isn't this searching all go's in the file which could cause a problem right?
True but then this becomes into an issue of making sure that all go version instances are matching. I would suppose that the maintainers make sure that their go version throughout the repo matches and if that's the case, then the go version that the nix flake pulls will be consistent, however we could add a section to the makefile that checks all instances of go versions as a check.
There was a problem hiding this comment.
A stricter regex that still works while ignoring the patch part of the go version would be:
^go\s(\d+\.\d+)\.\d+$There was a problem hiding this comment.
Yeah that could work, but only problem isn't this searching all go's in the file which could cause a problem right?
True but then this becomes into an issue of making sure that all go version instances are matching. I would suppose that the maintainers make sure that their go version throughout the repo matches and if that's the case, then the go version that the nix flake pulls will be consistent, however we could add a section to the makefile that checks all instances of go versions as a check.
Not really sure what you are talking about here. I wouldn't worry about any other go versions in other files (e.g. ci.yml) as part of this PR. We already need to make sure that these all agree when we bump the go version, and it's easy enough to find them all with a search.
For this PR, the only concern was that the regex is too loose and might match multiple hits in go.mod, and using a stricter regex solves that.
A stricter regex that still works while ignoring the patch part of the go version would be:
^go\s(\d+\.\d+)\.\d+$
Yes, this is almost the same as the one I was suggesting above (except for the + after the \s, which I don't care too much about as long as we have error checking). Still think you need to double the backslashes to escape them in a nix string literal.
Sounds like we agree on the approach and you can go ahead with this.
I agree to most of this this pr is at the point where you need to squash the commits. Second Maintaining the flake should be left up to the nix contributors and not the main program maintainers it should be in the official repo because packages take a few days before they reach unstable but have a way of getting the package early before unstable releases/updates. |
That alone is not a reason to host them here though, is it? The same can be said about any distribution or package manager. Look at this table, where most table cells need a few days to become green after a new release. We are not going to start hosting deb files, rpm spec files, or homebrew formula files in this repo to avoid that. |
Homebrew it's instant after pr release idk about others probably the same, but nixpkgs you have something called channels (stable and unstable) and channels get merged every couple of days so you have the time of the pr created till merged + time for channels which can equate to about two weeks. Which for many people you don't want to be waiting for the channel to have the change so many people upstream flakes to get the change instantly while also providing a development shell for those who use nix and want to develop they can. |
doprz
force-pushed
the
feature/nix-flake
branch
from
2f2e918 to
293b5a7
Compare
|
This configuration is at the point where it should not require any major changes or foreseeable maintenance to it. The main purpose of this PR was to allow nix users to have a development shell with all of the required deps and tools to start developing and using The original commit I made worked albeit not using best practices as I wasn't aware of things such as 
The PR is not about replacing the "unofficial" nix flake or adding distribution/package management files to the lazygit repo. It's about allowing nix users to have a declarative and reproducible dev environment to allow them to contribute to lazygit. All they have to do is cd into the repo and load up the dev shell or build lazygit via nix or with make. It's a quality of life PR at it's core. |
feat(nix): add nix run and build support chore: move vendor/ -> vendor_lg/ fix: revert vendor_lg -> vendor fix: fix vendor chore: set vendorHash to null docs: add local nix flake installation and contributing refactor(nix): use flake-parts, nix-systems, and flake-compat docs: remove nix profile installation section fix(nix): remove apps and hydraJobs fix: version info to match main chore: remove shellHook feat(nix): add treefmt-nix feat(nix): add gofmt to treefmt chore(nix): move version and gitCommit Apply suggestion from @Eveeifyeve Co-authored-by: Eveeifyeve <[email protected]> Apply suggestion from @Eveeifyeve Co-authored-by: Eveeifyeve <[email protected]> Apply suggestion from @Eveeifyeve Co-authored-by: Eveeifyeve <[email protected]> Apply suggestion from @Eveeifyeve Co-authored-by: Eveeifyeve <[email protected]> Update CONTRIBUTING.md Co-authored-by: Eveeifyeve <[email protected]> Update flake.nix Co-authored-by: Eveeifyeve <[email protected]> chore: change version to dev
doprz
force-pushed
the
feature/nix-flake
branch
from
293b5a7 to
516faea
Compare
| # Go toolchain | ||
| go | ||
| gotools | ||
| golangci-lint |
There was a problem hiding this comment.
For golangci-lint it is even more important than for go itself to be on the right version. Which checks are enabled by default tends to change between versions, so you are likely to get spurious linter warnings when using the latest version.
We already have a mechanism in place to mitigate this by using a shim that runs golangci-lint with the right version (see make lint in the Makefile), so I wonder if we should remove this entirely rather than add an explicit version.
There was a problem hiding this comment.
Got it. Yes, then we can remove this from the nix file as I'm including gnumake. Will wait for unresolved comments to be updated before pushing a chore commit and rebasing.
PR Description
This PR adds Nix flake support to provide a reproducible development environment and build process for
lazygitcontributors, devs, and users.Features:
nix develop,nix run, andnix buildCloses #3474.
Please check if the PR fulfills these requirements
n/a as this PR doesn't modify go files.
go generate ./...)