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

fix: build with go 1.23 by tracking modules.txt #168

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

obreitwi
Copy link

@obreitwi obreitwi commented Sep 1, 2024

Fixes: #117

Since go 1.23, vendor/modules.txt is required.

Hence, this is a first attempt to track modules.txt via gomod2nix.

Since, unfortunately, module.txt employs a bespoke custom format in which
varying numbers of # hold meaning, I deemed it more stable to not reverse-engineer it.

Instead, we generate it when running gomod2nix by executing go mod vendor and tracking its content in gomod2nix.toml.

Not the most elegant solution, but it works and enables us to use gomod2nix with go v1.23.

Any comments appreached…

Copy link

@hannesg hannesg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it locally and it indeed works with go 1.23.

@hannesg
Copy link

hannesg commented Sep 22, 2024

I checked why the unit test for the cli-args fails and the situation is somewhat complicated.

  1. go build -mod=vendor is now checking that imports are in vendor/modules.txt.
  2. The import that is missing in the build ( github.com/yuin/goldmark ) is in the go.mod of golang.org/x/tools but not in the vendor/modules.txt we generate.
  3. It's not in our vendor/modules.txt because the test only builds a subpackage that doesn't use it. Go can detect this because we create our own temporary module and only reference the subpackage there. The resulting go.mod is
module gomod2nix/dummy/package

go 1.23.1

require golang.org/x/tools v0.1.11

require (
        golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect
        golang.org/x/sys v0.0.0-20211019181941-9d821ace8654 // indirect
)

and so the resulting vendor/modules.txt is

# golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4
## explicit; go 1.17
golang.org/x/mod/semver
# golang.org/x/sys v0.0.0-20211019181941-9d821ace8654
## explicit; go 1.17
golang.org/x/sys/execabs
# golang.org/x/tools v0.1.11
## explicit; go 1.17
golang.org/x/tools/cmd/stringer
golang.org/x/tools/go/gcexportdata
golang.org/x/tools/go/internal/gcimporter
golang.org/x/tools/go/internal/packagesdriver
golang.org/x/tools/go/packages
golang.org/x/tools/internal/event
golang.org/x/tools/internal/event/core
golang.org/x/tools/internal/event/keys
golang.org/x/tools/internal/event/label
golang.org/x/tools/internal/gocommand
golang.org/x/tools/internal/packagesinternal
golang.org/x/tools/internal/typeparams
golang.org/x/tools/internal/typesinternal

This is absolutely correct. If you run go build -mod=vendor golang.org/x/tools/cmd/stringer on that, it works.

  1. So why does go then later claim that it's missing? That's because we are not building our temporary go module but from the source of golang.org/x/tools which does include the dependency in its go.mod.

So, how to fix this? I think ultimately we would need to bring the environment that generates the gomod2nix.toml and the environment that consumes it closer together which is tedious :(

@terlar
Copy link

terlar commented Nov 11, 2024

I tested this branch, and it seems it broke the --dir flag. Whatever is put in the --dir flag will be repeated in front of the downloaded vendor-gomod2nix, so inside the dir, you will have $DIR/$DIR/vendor-gomod2nix. Then you get a panic when it tries to read the modules.txt file which tries to read in the correct location.

panic: error generating pkgs: open $DIR/vendor-gomod2nix/modules.txt: no such file or directory

Fixes: nix-community#117

Since go 1.23, `vendor/modules.txt` is [required](golang/go@38ee0c7#diff-61fb6e44eac25bd4d6a8a64b3f38ee8a41faaefd1ef481170a011ecfc0f7c76bR344).

Hence, this is a first attempt to track `modules.txt` via gomod2nix.

Since, unfortunately, `module.txt` employs a bespoke custom format in
which
[varying](https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/cmd/go/internal/modload/vendor.go;l=57)
[numbers](https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/cmd/go/internal/modload/vendor.go;l=103)
of `#` hold meaning I deemed it more stable to not reverse engineer it.

Instead, we generate it when running `gomod2nix` by executing `go mod
vendor` and tracking its content in `gomod2nix.toml`.

Not the most elegant solution, but it works and enables us to use
gomod2nix with go v1.23.

Any comments appreached…
@yihuang
Copy link
Contributor

yihuang commented Dec 7, 2024

have been using this branch for a while, seems working fine 👍

Ran the following to fix the nix build

```
nix flake update
go mod tidy
go run . generate
nix build
```
@ghthor
Copy link

ghthor commented Dec 7, 2024

@obreitwi I think I fixed the nix build which should help the CI pass here if you merge my PR into your branch that this PR is based on.

  1. nix: fix build obreitwi/gomod2nix#1

@terlar
Copy link

terlar commented Dec 8, 2024

For more readable diffs it would be nice if the generated vendor modules txt was using a multiline """ string instead of a single line string with \n.

@obreitwi
Copy link
Author

obreitwi commented Dec 9, 2024

For more readable diffs it would be nice if the generated vendor modules txt was using a multiline """ string instead of a single line string with \n.

While I also prefer it, this would require a change of toml encoder since encoding multi-line strings is still an open issue.

@terlar
Copy link

terlar commented Dec 9, 2024

Oh, I throught it was already supported when looking at this comment:
toml-lang/toml#163 (comment)

@yihuang
Copy link
Contributor

yihuang commented Dec 10, 2024

For more readable diffs it would be nice if the generated vendor modules txt was using a multiline """ string instead of a single line string with \n.

While I also prefer it, this would require a change of toml encoder since encoding multi-line strings is still an open issue.

can we split it by \n and encode it as a list. it's also encoded in the same line.

@obreitwi
Copy link
Author

it's also encoded in the same line.

Can confirm (didn't see your edit beforehand). I think in the end it comes down to switching to another toml encoder (or contributing the functionality upstream).

@splack
Copy link

splack commented Jan 1, 2025

There are a number of issues and PRs turning up related to this. Given the length of time the toml package has had a fix for the multi line pending in it's large rewrite branch, it seems like the toml package will not be released anytime soon. Switching to another toml package might be better done in another PR first, but would be easier if using at least a go version that is in stable nixpkgs, e.g. go_1_22.

What needs to be done to get this merged?

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.

Make mkVendorEnv generate modules.txt
6 participants