golangci-lint: build with buildGo123Module
#339057
Conversation
Signed-off-by: budimanjojo <budimanjojo@gmail.com>
|
Tagging @SuperSandro2000 and @Mic92 as the maintainer of the package |
|
Not sure why the CI failed, the log looks unrelated to this PR too. |
|
@ofborg eval |
| @@ -1,6 +1,6 @@ | |||
| { buildGoModule, fetchFromGitHub, lib, installShellFiles }: | |||
| { buildGo123Module, fetchFromGitHub, lib, installShellFiles }: | |||
There was a problem hiding this comment.
Are there any other projects that are affected by this in the same way?
A buildGoModule_latest would be useful in that instance.
There was a problem hiding this comment.
I think gopls is another one that wants latest available Go as much as possible.
There was a problem hiding this comment.
I also think we should always use latest available go to build packages, so maybe default buildGoModule to latest version, and for packages that don't like it can use something like buildGoModule_stable or the currently available buildGo122Module?
There was a problem hiding this comment.
That could delay adding new go compilers significantly because now go release maintainers have to upgrade the ecosystem in one go. In the end you would be in a worse position for your projects because there would be no new go compiler to switch to in nixpkgs until everything is updated.
There was a problem hiding this comment.
Ah okay, I don't really understand how things work in nixpkgs tbh so you might have the better answer than me. For now the most important thing for me is golangci-lint and gopls because it has a really nasty habit of freezing my system with all RAM and CPU maxed out whenever I open my LSP enabled Neovim on a project with latest go.mod file so I think these two should always use latest available Go version when possible.
| @@ -1,6 +1,6 @@ | |||
| { buildGoModule, fetchFromGitHub, lib, installShellFiles }: | |||
| { buildGo123Module, fetchFromGitHub, lib, installShellFiles }: | |||
There was a problem hiding this comment.
such overrides should be done in all-packages.nix
/~https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#changing-implicit-attribute-defaults
i know these docs are for by-name, but same argument applies
There was a problem hiding this comment.
IMO this doesn't really matter
There was a problem hiding this comment.
in my local project, I override buildGoModule for gopls, golangcl-lint and few others, each time I update nixpkgs, I have to fix arguments, because callPackage augments keep changing
There was a problem hiding this comment.
Actually @kirillrdy is kinda right. Right now I have this in my overlay:
(final:prev: {
gopls = prev.gopls.override { buildGoModule = prev.buildGo123Module; };
})
If this got into unstable, then my override will not work anymore. But I also think that if it's done in all-packages.nix, it will be hard to find out which go version is being used because all-packages.nix is so hard to look at in browser because of the file size.
There was a problem hiding this comment.
Having buildGoModule_latest or some sort like suggested by @Mic92 is the way to go IMO.
There was a problem hiding this comment.
Afaik go has a principle of backward compatibility and using latest go version to build packages should do no harm? Maybe I'm wrong but I never have issue building ancient packages using latest go version.
Also, we have full control on when to bump buildGoModule_latest up not necessarily on day one of new go release?
There was a problem hiding this comment.
For normal packages yes, go is backwards compatible, but packages that deeply go into the compile process like linters may compile but sometimes don't properly work and need a fix because some internal changed.
Also if we need to wait for a few days and updates to some packages, the point of a buildGoModule_latest twingles.
There was a problem hiding this comment.
What about something like that?
| { buildGo123Module, fetchFromGitHub, lib, installShellFiles }: | |
| { buildGo123Module | |
| , fetchFromGitHub | |
| , lib | |
| , installShellFiles | |
| , builder ? buildGo123Module | |
| }: |
and then users can override builder (or whatever we decide to name it) without worrying about it being renamed and everything remain in this file:
(final:prev: {
golangci-lint = prev.golangci-lint.override { builder = prev.buildGo124Module; };
})There was a problem hiding this comment.
Seems to be lots of different ways to achieve this! I want to submit a pr to do the same for govulncheck to build it with latest go for the same reason as this pr, but I'm not sure which option to choose.
There was a problem hiding this comment.
@maxbrunet that is equal to overriding buildGoModule in all-packages.
Description of changes
Build
golangci-lintwithgo_1_23.Currently, the
golangci-lintrefuses to run when the project is using higher Go version than the version used to build the program (see golangci/golangci-lint#4938).This was because of a severe memory leak issue when trying to run
golangci-linton go 1.23 project.There's no point of still building it with older version of Go since
golangci-lintalready support it since 1.60.1.Before this change:
After this change:
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.