gomod2nix: init at 1.5.0 #188272

adisbladis · 2022-08-25T08:28:08Z

Description of changes

This adds gomod2nix, a generator for Nix expressions using Go modules (and the associated build infrastructure).
It has quite some differences to buildGoModule which are laid out in https://www.tweag.io/blog/2021-03-04-gomod2nix/.

I don't feel quite ready yet to promote buildGoApplication and mkGoEnv to first-class top-level attributes so in nixpkgs these will be accessed by gomod2nix.buildGoApplication & gomod2nix.mkGoEnv respectively.

This is developed external over at https://github.com/tweag/gomod2nix/.

This work has been sponsored by both the NLNet Foundation under the NGI Zero program and by Tweag.

Due to a mistake this is a new instance of #188035. See that PR for previous discussion.

Things done

adisbladis · 2022-08-25T10:53:06Z

Repating myself from: #188035 (comment)

I intend to merge this within the next couple of days unless there are any objections.

zowoq · 2022-08-25T11:29:49Z

The update script seems broken, doesn't seem to eval. I said previously I won't block on this so if you just want to drop it from this PR and deal with it in a follow up I'm fine with that.

Can you share your plans and time frame for promoting for this?

adisbladis · 2022-08-25T12:56:59Z

The update script seems broken, doesn't seem to eval. I said previously I won't block on this so if you just want to drop it from this PR and deal with it in a follow up I'm fine with that.

I made a fix to make the updater work in the case that pname isn't explicitly passed.

Can you share your plans and time frame for promoting for this?

I have patches ready for all the Go packages I maintain in nixpkgs and will announce the availability of gomod2nix in nixpkgs shortly after at least a few of those have been merged.
I will however not try to go on some crusade to get everyone to adopt it immediately.

Additionally I also have patches for most of the packages that still use vgo2nix ready too, it's just blocked on this PR.

adisbladis · 2022-08-25T13:17:37Z

@zowoq FYI the upstream repo has been moved to nix-community.

zowoq · 2022-08-25T13:24:32Z

I think the case-sensitive problems in #129730 may also be a problem for this?

I made a fix to make the updater work in the case that pname isn't explicitly passed.

Sorry, still doesn't seem to work. Can you add an example to this PR?

I have patches ready for all the Go packages I maintain in nixpkgs

Not any of the podman packages please.

will announce the availability of gomod2nix in nixpkgs shortly after at least a few of those have been merged.

Can we wait at least a couple weeks on this? I'd like to do some testing and fix some nits first. I really don't see why this needs to be rushed, at one point I thought the upstream project had been abandoned. IIRC the go no vendor patch was added 18 months ago.

FYI the upstream repo has been moved to nix-community.

Cool, thanks.

adisbladis · 2022-08-25T13:52:40Z

Can we wait at least a couple weeks on this? I'd like to do some testing and fix some nits first. I really don't see why this needs to be rushed

Sure that could wait, no problem.

Not any of the podman packages please.

I was talking about the ones where I'm the sole maintainer,.

I think the case-sensitive problems in #129730 may also be a problem for this?

I don't think so, I could add a test case for it though.

Sorry, still doesn't seem to work. Can you add an example to this PR?

Sure, I've added repackaged a fairly low impact package in this PR.
It was generated by running gomod2nix generate github.com/google/trillian/cmd/trillian_log_server github.com/google/trillian/cmd/trillian_log_signer github.com/google/trillian/cmd/createtree github.com/google/trillian/cmd/deletetree github.com/google/trillian/cmd/updatetree.

zowoq · 2022-08-25T14:03:52Z

pkgs/tools/misc/trillian/gomod2nix.toml

@@ -0,0 +1,167 @@
+schema = 3
+subPackages = ["cmd/trillian_log_server", "cmd/trillian_log_signer", "cmd/createtree", "cmd/deletetree", "cmd/updatetree"]


Why does this need to be moved?

It's not moved per se, you can still set the same attribute in the Nix expression.
This is just another way to set the same thing which is a lot easier to control from the code generator.

but why do we need to pass this through the code generator? This often needs to be modify to build additional tools or not build testing tools. For me this belongs into the nix-expression and not in the generated code.

For one it serves to reduce the build time closure size.
We don't actually need everything in the root level go.mod and by making the generator aware of what needs to be built we only need to pull in the actually required subset of dependencies.

zowoq · 2022-08-25T14:03:54Z

pkgs/tools/misc/trillian/gomod2nix.toml

+subPackages = ["cmd/trillian_log_server", "cmd/trillian_log_signer", "cmd/createtree", "cmd/deletetree", "cmd/updatetree"]
+goPackagePath = "github.com/google/trillian"


Why is this needed?

Why can't we infer this from the src?

Why does it need to be in this file?

There is no src attribute explicitly being set.
The goPackagePath is there so the derivation knows which sources to select for the src attribute.

You could separate the src if you wanted to and do the whole manual hash update dance, nothing stops you from doing that but that will leave you without an update script.

There is no src attribute explicitly being set.

I am not really a fan of this. If I want to access the source files nix is building things from to investigate and debug packages I often use nix build .#name.src and I would tell people to do that when debugging. Could we add it at least as an alias back to make things more uniform with other packages?

Also we need to make sure that it is clear in the documentation that goPackagePath here has nothing to do with buildGoPackage.

BTW what is your plan for documentation?

zowoq · 2022-08-25T14:11:19Z

oh the whole thing gets stuffed into the toml. src, version and everything. TBH I really don't like this:

{ lib
, gomod2nix
, fetchFromGitHub
}:

gomod2nix.buildGoApplication {
  pname = "trillian";
  pwd = ./.;
  meta = {
    homepage = "https://github.com/google/trillian";
    description = "A transparent, highly scalable and cryptographically verifiable data store.";
    license = [ lib.licenses.asl20 ];
    maintainers = [ lib.maintainers.adisbladis ];
  };
}

adisbladis · 2022-08-25T14:21:13Z

oh the whole thing gets stuffed into the toml. src, version and everything. TBH I really don't like this:

You could rewrite it in a more verbose way if you wanted to. Look at how https://github.com/nix-community/gomod2nix/blob/master/generic.nix or https://github.com/crypto-org-chain/chain-main/blob/master/default.nix are constructed for example.

There are very good reasons to do it this way though. In buildGoModule src and vendorSha256 requires you to update multiple hashes which is absolutely terrible UX.
I didn't want to repeat this terrible UX and made the code generated file the only thing you need to update as a single atomic unit.

Otherwise we'd have a similarly bad user experience once again for very little reason.

zowoq · 2022-08-25T14:24:09Z

To get the package version I either need to query the package via nix or grep the toml?

adisbladis · 2022-08-25T14:28:04Z

To get the package version I either need to query the package via nix or grep the toml?

In this instance, yes.

zowoq · 2022-08-25T14:30:12Z

Sorry but I think burying the version in the toml is absolutely horrible.

adisbladis · 2022-08-25T14:36:01Z

Sorry but I think burying the version in the toml is absolutely horrible.

Ok, I don't see the problem. There is plenty of prior art in nixpkgs already.

zowoq · 2022-08-25T14:49:12Z

There is plenty of prior art in nixpkgs already.

Package sets or separate packages like this?

I don't see the problem.

Searching "github.com/google/trillian" to find the version isn't a problem? Seems like we're trading one form of bad UX for another?

adisbladis · 2022-08-25T14:54:15Z

Package sets or separate packages like this?

Both. Ruby packages comes to mind.

Searching "github.com/google/trillian" to find the version isn't a problem? Seems like we're trading one form of bad UX for another?

Would you feel better if we also stuck the version attribute in the top-level attributes in the toml file?

zowoq · 2022-08-25T15:22:12Z

Ruby packages comes to mind.

If you mean bundlerEnv with gemset.nix and gemfile.lock that seems very different. Any others? I'm actually curious as I can't think of any. haskell, emacs, vim, node all seem much easier to inspect visually.

Would you feel better if we also stuck the version attribute in the top-level attributes in the toml file?

Only marginally. Would need to be enforced, I really don't want debates over it and packages flipping back and forth between (buried in toml|top of toml|set in default.nix).

You could rewrite it in a more verbose way if you wanted to. Look at how https://github.com/nix-community/gomod2nix/blob/master/generic.nix or https://github.com/crypto-org-chain/chain-main/blob/master/default.nix are constructed for example.

I'm kinda worried about the fact that this is possible. At the moment there is (basically) one way of writing a go package, this seems to allow a lot more variation (and more debates). Consistency is good UX also.

adisbladis · 2022-08-25T15:39:02Z

Any others? I'm actually curious as I can't think of any. haskell, emacs, vim, node all seem much easier to inspect visually.

Carnix is another one (code generator), so is poetry2nix (reads pyproject.toml).

I think it's disingenuous to say that Emacs is easier to inspect visually, the majority of that lives in massive auto generated files. The same goes for Haskell, Node & many more.

My take is that this whole version in the nix file or not comment is just bike shedding with extremely small practical impact.

I'm kinda worried about the fact that this is possible. At the moment there is (basically) one way of writing a go package, this seems to allow a lot more variation (and more debates).

Which is intentional. The needs of a packager packaging up an application in a private repo has somewhat different concerns than someone packaging something that is developed entirely outside of the package tree.

buildGoModule lacks this flexibility which (imo) makes it unusable as a tool to integrate into a typical development workflow.

gomod2nix was designed with both use cases in mind.

zowoq · 2022-08-25T17:34:18Z

Carnix is another one (code generator)

nixpkgs/pkgs/tools/toml2nix/default.nix

Line 9 in 6943487

toml2nix = crates.crates.toml2nix."0.1.1" deps;

so is poetry2nix (reads pyproject.toml).

nixpkgs/pkgs/tools/package-management/nixops/default.nix

Line 26 in 6943487

version = "1.7";

Not familiar with these but at a glance they both seem pretty close to a standard package?

I think it's disingenuous to say that Emacs is easier to inspect visually, the majority of that lives in massive auto generated files. The same goes for Haskell, Node & many more.

They all seem to declare a version that isn't mixed in with it's own deps?

My take is that this whole version in the nix file or not comment is just bike shedding with extremely small practical impact.

The needs of a packager packaging up an application in a private repo has somewhat different concerns than someone packaging something that is developed entirely outside of the package tree.

buildGoModule lacks this flexibility which (imo) makes it unusable as a tool to integrate into a typical development workflow.

I'll response to these all at once as it's basically the same point:

Yeah, I agree that buildGoModule isn't fit for dev use outside of nixpkgs and yes, I agree that different packagers have different needs. For packages that are in nixpkgs we don't need (and IMO really don't want) a more complex tool that can be used in multiple ways/styles. We have ~1200 go packages and we add more each day. Allowing go packaging to become more complicated and more prone to debate doesn't make the lives of the go maintainers easier. Same applies for people reviewing go package PRs.

Just to be clear: I'm not objecting to using gomod2nix in nixpkgs, I'd just like to only allow one style to be used in nixpkgs that covers all but extreme cases so we have less bike shedding.

SuperSandro2000 · 2022-08-30T13:00:44Z

Did you do some testing with big go.mod files and how they behave like https://github.com/golangci/golangci-lint/blob/master/go.mod and more complex ones like https://github.com/open-policy-agent/gatekeeper/blob/master/go.mod ? Also what if the project uses replace where we can't use go install with the package name?

SuperSandro2000 · 2022-08-30T13:20:44Z

oh the whole thing gets stuffed into the toml. src, version and everything. TBH I really don't like this:

same for me. This is not very great from a reviewers perspective who needs to double check that people did not forgot to update version or some hash.

FRidh · 2022-08-30T13:22:03Z

Glad to see a solution without vendorSha256!

SuperSandro2000

I am sorry that we are doing another review round but I'd rather do this correct the first time than fixing it up multiple times and potentially breaking already packaged packages with it.

pkgs/build-support/go/gomod2nix/parser.nix

pkgs/build-support/go/gomod2nix/symlink/symlink.go

pkgs/development/tools/gomod2nix/generic.nix

pkgs/top-level/all-packages.nix

pkgs/build-support/go/gomod2nix/default.nix

pkgs/build-support/go/gomod2nix/symlink/symlink.go

pkgs/tools/misc/trillian/gomod2nix.toml

pkgs/build-support/go/gomod2nix/default.nix

…ore readable Implement suggestion from NixOS/nixpkgs#188272 (comment).

adisbladis · 2022-09-06T21:04:45Z

Trying this again: If there is nothing new and actionable by Friday (2022-09-09) I will merge this.

zowoq · 2022-09-07T07:26:14Z

Trying this again: If there is nothing new and actionable by Friday (2022-09-09) I will merge this.

You haven’t responded to a few concerns from Sandro and myself. Between us we’ve done the majority of the go maintenance for the last year so I don’t think our concerns should be dismissed.

There is no src attribute explicitly being set.

I am not really a fan of this. If I want to access the source files nix is building things from to investigate and debug packages I often use nix build .#name.src and I would tell people to do that when debugging. Could we add it at least as an alias back to make things more uniform with other packages?

I don’t think this has been addressed?

Also we need to make sure that it is clear in the documentation that goPackagePath here has nothing to do with buildGoPackage.

Renaming goPackagePath to goModPath, goSrcPath or something else would eliminate any confusion with buildGoPackage.

what is your plan for documentation?

I don’t mind if this PR is merged without docs but I think having docs is a blocker for gomod2nix, etc being promoted.

oh the whole thing gets stuffed into the toml. src, version and everything. TBH I really don't like this:

same for me. This is not very great from a reviewers perspective who needs to double check that people did not forgot to update version or some hash.

This hasn’t been addressed. As you asked for something actionable: I’d like to see the src/hash/version not treated as just another dep and be moved to the top of the toml with the other attributes.

I consider this a blocker for merging this PR.

Did you do some testing with big go.mod files and how they behave like https://github.com/golangci/golangci-lint/blob/master/go.mod and more complex ones like https://github.com/open-policy-agent/gatekeeper/blob/master/go.mod ? Also what if the project uses replace where we can't use go install with the package name?

Sandro's asked about this twice (in this PR and #188035 (comment)) and there doesn't seem to have been a response?

06kellyjac · 2022-09-07T08:13:02Z

As someone who's also been packaging go modules a bunch I agree with all @zowoq & @SuperSandro2000 's points in that comment

I'll try this out with cosign which has a bajillion dependencies

06kellyjac · 2022-09-07T08:51:44Z

pkgs/build-support/go/gomod2nix/default.nix

+ symlink = mkInternalPkg "symlink" ./symlink/symlink.go;
+
+ # Install development dependencies from tools.go
+ install = mkInternalPkg "symlink" ./install/install.go;


Suggested change

install = mkInternalPkg "symlink" ./install/install.go;

install = mkInternalPkg "install" ./install/install.go;

06kellyjac · 2022-09-07T10:05:29Z

pkgs/build-support/go/gomod2nix/install/install.go

+ "strconv"
+)
+
+const filename = "tools.go"


Is it actually enforced that the file is called tools.go or just that the build tag is tools?

https://marcofranssen.nl/manage-go-tools-via-go-modules

// +build tools

I can't really find any docs on this

adisbladis requested review from kalbasit, Mic92 and zowoq as code owners August 25, 2022 08:28

github-actions bot added the 6.topic: golang label Aug 25, 2022

adisbladis mentioned this pull request Aug 25, 2022

gomod2nix: init at 1.3.0 #188035

Merged

13 tasks

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 25, 2022

adisbladis force-pushed the gomod2nix-1_4_0 branch from b43e169 to c2049d5 Compare August 25, 2022 12:53

ofborg bot removed 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Aug 25, 2022

zowoq reviewed Aug 25, 2022

View reviewed changes

SuperSandro2000 reviewed Aug 30, 2022

View reviewed changes

06kellyjac reviewed Aug 30, 2022

View reviewed changes

pkgs/build-support/go/gomod2nix/symlink/symlink.go Show resolved Hide resolved

06kellyjac reviewed Aug 30, 2022

View reviewed changes

pkgs/tools/misc/trillian/gomod2nix.toml Show resolved Hide resolved

06kellyjac reviewed Aug 30, 2022

View reviewed changes

pkgs/build-support/go/gomod2nix/default.nix Outdated Show resolved Hide resolved

06kellyjac reviewed Aug 30, 2022

View reviewed changes

pkgs/build-support/go/gomod2nix/default.nix Outdated Show resolved Hide resolved

adisbladis added a commit to nix-community/gomod2nix that referenced this pull request Sep 2, 2022

builder/symlink: Factor out into separate functions to make indents m…

9b360fe

…ore readable Implement suggestion from NixOS/nixpkgs#188272 (comment).

adisbladis mentioned this pull request Sep 2, 2022

Symlink indent dehadouken nix-community/gomod2nix#86

Merged

adisbladis force-pushed the gomod2nix-1_4_0 branch 2 times, most recently from d257851 to 06e13d1 Compare September 2, 2022 05:36

adisbladis changed the title ~~gomod2nix: init at 1.4.0~~ gomod2nix: init at 1.5.0 Sep 2, 2022

adisbladis force-pushed the gomod2nix-1_4_0 branch from 06e13d1 to b83aa88 Compare September 2, 2022 05:50

adisbladis added 2 commits September 2, 2022 17:52

gomod2nix: init at 1.5.0

be8355f

trillian: re-package using gomod2nix

a69d230

adisbladis force-pushed the gomod2nix-1_4_0 branch from b83aa88 to a69d230 Compare September 2, 2022 05:52

06kellyjac reviewed Sep 7, 2022

View reviewed changes

wegank marked this pull request as draft October 1, 2023 01:46

adisbladis closed this Feb 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gomod2nix: init at 1.5.0 #188272

gomod2nix: init at 1.5.0 #188272

adisbladis commented Aug 25, 2022 •

edited

Loading

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022 •

edited

Loading

zowoq Aug 25, 2022

adisbladis Aug 25, 2022

SuperSandro2000 Aug 30, 2022

adisbladis Sep 2, 2022

zowoq Aug 25, 2022

adisbladis Aug 25, 2022

SuperSandro2000 Aug 30, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022 •

edited

Loading

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022 •

edited

Loading

zowoq commented Aug 25, 2022

SuperSandro2000 commented Aug 30, 2022

SuperSandro2000 commented Aug 30, 2022

FRidh commented Aug 30, 2022

SuperSandro2000 left a comment

adisbladis commented Sep 6, 2022

zowoq commented Sep 7, 2022

06kellyjac commented Sep 7, 2022

06kellyjac Sep 7, 2022

06kellyjac Sep 7, 2022

		@@ -0,0 +1,167 @@
		schema = 3
		subPackages = ["cmd/trillian_log_server", "cmd/trillian_log_signer", "cmd/createtree", "cmd/deletetree", "cmd/updatetree"]

		subPackages = ["cmd/trillian_log_server", "cmd/trillian_log_signer", "cmd/createtree", "cmd/deletetree", "cmd/updatetree"]
		goPackagePath = "github.com/google/trillian"

	install = mkInternalPkg "symlink" ./install/install.go;
	install = mkInternalPkg "install" ./install/install.go;

gomod2nix: init at 1.5.0 #188272

gomod2nix: init at 1.5.0 #188272

Conversation

adisbladis commented Aug 25, 2022 • edited Loading

Description of changes

Things done

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022 • edited Loading

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022

zowoq commented Aug 25, 2022

adisbladis commented Aug 25, 2022 • edited Loading

zowoq commented Aug 25, 2022

SuperSandro2000 commented Aug 30, 2022

SuperSandro2000 commented Aug 30, 2022

FRidh commented Aug 30, 2022

SuperSandro2000 left a comment

Choose a reason for hiding this comment

adisbladis commented Sep 6, 2022

zowoq commented Sep 7, 2022

06kellyjac commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisbladis commented Aug 25, 2022 •

edited

Loading

adisbladis commented Aug 25, 2022 •

edited

Loading

adisbladis commented Aug 25, 2022 •

edited

Loading

adisbladis commented Aug 25, 2022 •

edited

Loading