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

Build depends on an apparently non-existent toml 0.3.2 version #74

Closed
dmbaturin opened this issue Jun 28, 2021 · 21 comments
Closed

Build depends on an apparently non-existent toml 0.3.2 version #74

dmbaturin opened this issue Jun 28, 2021 · 21 comments

Comments

@dmbaturin
Copy link
Contributor

I wanted to use the test suite to test my work-in-progress TOML library, but the build fails with this error for me:

$ go build
go: github.com/BurntSushi/toml@v0.3.2-0.20210621044154-20a94d639b8e: invalid version: unknown revision 20a94d639b8e

@arp242 arp242 closed this as completed in 7327aef Jun 28, 2021
@arp242
Copy link
Collaborator

arp242 commented Jun 28, 2021

The 0.3.2 is a "fake" version it puts it when you use a specific commit (20a94d639b8e).

That commit indeed doesn't exist, but it does build on my system as I have toml@v0.3.2-0.20210621044154-20a94d639b8e in my mod cache. I think it ended up there with some replace directives and/or git commit --amend during development(?) What a sneaky issue! Should probably set up a basic GitHub action runner for this repo.

Anyway, I fixed it now!

@dmbaturin
Copy link
Contributor Author

Sorry, it still doesn't build from a freshly-cloned repo for me.

$ go build
go: github.com/BurntSushi/toml@v0.3.2-0.20210624061728-01bfc69d1057 requires
	github.com/BurntSushi/toml-test@v0.1.1-0.20210624055653-1f6389604dc6 requires
	github.com/BurntSushi/toml@v0.3.2-0.20210621044154-20a94d639b8e: invalid version: unknown revision 20a94d639b8e

$ git log
commit 7327aef73d079f5f890c5e067a5ad2d63d6b7671 (HEAD -> master, origin/master, origin/HEAD)
Author: someone else <martin@arp242.net>
Date:   Mon Jun 28 22:31:33 2021 +0800

    Update toml version
    
    Fixes #74

@arp242
Copy link
Collaborator

arp242 commented Jun 28, 2021

Ehm; I'm not sure, I cleaned up all the cache on my system and it works there. I also set up GitHub actions where it seems to build fine: https://github.com/BurntSushi/toml-test/runs/2936504692

Which Go version are you using?

I also tagged 1.0.0-beta2 and published binaries for them, so you can use that too:
https://github.com/BurntSushi/toml-test/releases/tag/1.0.0-beta2

@dmbaturin
Copy link
Contributor Author

I'm using 1.16.4 from the Fedora 34 repos. Here's what exactly I've done and how it failed:

[dmbaturin@haruko ~]$ rm -rf ~/go/
[dmbaturin@haruko ~]$ rm -rf ~/toml-test/

[dmbaturin@haruko ~]$ git clone https://github.com/BurntSushi/toml-test.git
Cloning into 'toml-test'...
remote: Enumerating objects: 1340, done.
remote: Counting objects: 100% (387/387), done.
remote: Compressing objects: 100% (200/200), done.
remote: Total 1340 (delta 89), reused 325 (delta 55), pack-reused 953
Receiving objects: 100% (1340/1340), 236.44 KiB | 3.24 MiB/s, done.
Resolving deltas: 100% (497/497), done.

[dmbaturin@haruko ~]$ cd toml-test/
[dmbaturin@haruko toml-test]$ go build
go: github.com/BurntSushi/toml@v0.3.2-0.20210624061728-01bfc69d1057 requires
	github.com/BurntSushi/toml-test@v0.1.1-0.20210624055653-1f6389604dc6 requires
	github.com/BurntSushi/toml@v0.3.2-0.20210621044154-20a94d639b8e: invalid version: unknown revision 20a94d639b8e

[dmbaturin@haruko toml-test]$ go version
go version go1.16.4 linux/amd64

@arp242
Copy link
Collaborator

arp242 commented Jun 30, 2021

I don't know; it works for me and on GitHub actions. I know this sounds a bit lame, but I'm not sure where the problem is in your case 🤔

I'll reopen this and see if I can find out something later; as I mentioned, in the meanwhile you can use the binary builds.

@arp242 arp242 reopened this Jun 30, 2021
@dmbaturin
Copy link
Contributor Author

I'm going to try it on another knowingly clean setup some time. I've been using a binary build meanwhile, it works for me (though I'll need to get a build setup done to contribute).

@arp242
Copy link
Collaborator

arp242 commented Jul 4, 2021

Maybe try:

$ go clean -cache
$ go clean -modcache

Someone else mentioned they could install it without issue a few days ago too.

(You also need to use go build ./cmd/toml-test instead of go build by the way, not the cause of this error, just FYI)

tymonx added a commit to tymonx/toml-test that referenced this issue Aug 4, 2021
Build depends on an apparently non-existent toml 0.3.2 version.

If someone will use non-default Go proxy or direct only `GOPROXY=direct`,
the toml-test project will fail miserable.

It effects all projects around the world that uses the `toml` project
because it depends on the `toml-test` project.

This patch fixes it by setting correct SHA commit using
the `go mod tidy` command.
@arp242 arp242 closed this as completed in 27837d3 Aug 4, 2021
@tymonx
Copy link

tymonx commented Aug 4, 2021

Hi @arp242 This is not solved. It can be easily reproduced.

Run it in the official Go image:

docker run -it --rm golang:1.16

Set the GOPROXY environment variable:

export GOPROXY=direct

Simple download and build:

go get -u github.com/BurntSushi/toml-test

Output:

go: downloading github.com/BurntSushi/toml-test v1.0.0
go get: github.com/BurntSushi/toml-test@v0.1.0 updating to
        github.com/BurntSushi/toml-test@v1.0.0 requires
        github.com/BurntSushi/toml@v0.4.0 requires
        github.com/BurntSushi/toml-test@v0.1.1-0.20210723065233-facb9eccd4da requires
        github.com/BurntSushi/toml@v0.3.2-0.20210704081116-ccff24ee4463 requires
        github.com/BurntSushi/toml-test@v0.1.1-0.20210704062846-269931e74e3f requires
        github.com/BurntSushi/toml@v0.3.2-0.20210624061728-01bfc69d1057 requires
        github.com/BurntSushi/toml-test@v0.1.1-0.20210624055653-1f6389604dc6 requires
        github.com/BurntSushi/toml@v0.3.2-0.20210621044154-20a94d639b8e: invalid version: unknown revision 20a94d639b8e

This affects all projects that depends on the toml project. For example:

go get -u github.com/mgechev/revive

Output:

go get: github.com/BurntSushi/toml@v0.3.1 updating to
        github.com/BurntSushi/toml@v0.4.0 requires
        github.com/BurntSushi/toml-test@v0.1.1-0.20210723065233-facb9eccd4da requires
        github.com/BurntSushi/toml@v0.3.2-0.20210704081116-ccff24ee4463 requires
        github.com/BurntSushi/toml-test@v0.1.1-0.20210704062846-269931e74e3f requires
        github.com/BurntSushi/toml@v0.3.2-0.20210624061728-01bfc69d1057 requires
        github.com/BurntSushi/toml-test@v0.1.1-0.20210624055653-1f6389604dc6 requires
        github.com/BurntSushi/toml@v0.3.2-0.20210621044154-20a94d639b8e: invalid version: unknown revision 20a94d639b8e

@arp242
Copy link
Collaborator

arp242 commented Aug 4, 2021

I don't have Docker, but it indeed errors out with:

% export GOPATH=/home/martin/tmpgo
% export GOPROXY=direct

% go get -v github.com/BurntSushi/toml-test
go: downloading github.com/BurntSushi/toml-test v1.0.0
get "zgo.at/zli": found meta tag vcs.metaImport{Prefix:"zgo.at/zli", VCS:"git", RepoRoot:"https://github.com/zgoat/zli.git"} at //zgo.at/zli?go-get=1
go get: github.com/BurntSushi/toml-test@v0.1.0 updating to
		github.com/BurntSushi/toml-test@v1.0.0 requires
		github.com/BurntSushi/toml@v0.4.0 requires
		github.com/BurntSushi/toml-test@v0.1.1-0.20210723065233-facb9eccd4da requires
		github.com/BurntSushi/toml@v0.3.2-0.20210704081116-ccff24ee4463 requires
		github.com/BurntSushi/toml-test@v0.1.1-0.20210704062846-269931e74e3f requires
		github.com/BurntSushi/toml@v0.3.2-0.20210624061728-01bfc69d1057 requires
		github.com/BurntSushi/toml-test@v0.1.1-0.20210624055653-1f6389604dc6 requires
		github.com/BurntSushi/toml@v0.3.2-0.20210621044154-20a94d639b8e: invalid version: unknown revision 20a94d639b8e

Yet that revision does exist:

% cd ~/code/toml && git show 20a94d639b8e
commit 20a94d6
Author: Martin Tournoij <martin@arp242.net>
Date:   Sun Jun 20 01:04:37 2021 +0800

	Implement inline tables, too

[...]

This seems like a bug in Go(?) Not sure, but it seems very strange that it behaves different, and from a quick glance at the docs I don't see any reason it should.

This affects all projects that depends on the toml project. For example:

Only if you choose to update all dependencies with -u. You really shouldn't do that, but instead use the versions in the go.mod which is pinned to 0.3.1 in that project for example.

@dmbaturin
Copy link
Contributor Author

This is some truly mysterious stuff. I could build the executable with Go 1.16 installed from Google's tarball, but not with the same version of Go coming from Fedora repos. I also had the issue reproducible on with 1.15 from Debian Buster repos.

@tymonx
Copy link

tymonx commented Aug 4, 2021

You can check it with the go env GOPROXY command. I'm also using Fedora 34 Workstation and I have the same issue here. I'm also the author of the GitLab CI templates and now all Go projects that depends on the toml project in CI fails.

@arp242
Copy link
Collaborator

arp242 commented Aug 4, 2021

I suppose I can just remove the cyclic test dependency for now, although I wouldn't be a fan of it as the current method is quite useful in development, so it would be good to know what exactly is going on here.

If someone has the time and inclination to investigate this in some depth then that would be appreciated; I couldn't really find anything in the Go docs or issue tracker. Otherwise I can just comment it out in the toml repo for the time being and release v0.4.1 I suppose, as a pragmatic temporary fix.

I'm also the author of the GitLab CI templates and now all Go projects that depends on the toml project in CI fails.

You really should modify that template; getting a newer version in spite of a pinned version in go.mod is not good. That this doesn't work with GOPROXY=direct is something that's a problem either in this library or in Go itself, but that merely the tagging of a new version can break your CI is a bug in your CI template. As we've discovered here, accidental problems/mistakes/etc. happen, which is why there are tagged versions.

@arp242
Copy link
Collaborator

arp242 commented Aug 4, 2021

I'm also confused how your PR actually fixed things by the way, as it still refers the same commit/version, just in a roundabout way?

Another option might be some creative use of build tags 🤔

@arp242 arp242 reopened this Aug 4, 2021
@tymonx
Copy link

tymonx commented Aug 4, 2021

You really should modify that template

I'm aware of that. I will simple add an environment variable to allow users set the go get -u it and without it. Both cases are worldly to tests. More proper and stable solution is to prepare a Docker image with Go and all lint, formatting and test utilities. On default it will point to this image. Currently the script in Go CI template install all tools. This scenario is also handy. But because of that I was able to quickly find this issue. I will prepare it next week.

Removing the -u option doesn't solve anything. It is the same issue.

I'm also confused how your PR actually fixed things by the way, as it still refers the same commit/version, just in a roundabout way?

I was simple allow the go mod tidy command to the work.

Prepare simple go.mod:

module github.com/BurntSushi/toml-test

go 1.16

// no_term branch, which doesn't depend on x/term and x/sys
require zgo.at/zli v0.0.0-20210619044753-e7020a328e59

replace github.com/BurntSushi/toml => github.com/BurntSushi/toml v0.4.0

And run:

go mod tidy

The replace directive tells Go to replace all imports github.com/BurntSushi/toml with proper version and resolve it. It is because of cyclic dependency in your project toml-test -> toml -> toml-test -> ... You can see it in above output. This crazy cyclic dependency is resolved by the Go proxy https://proxy.golang.org but this proxy is not good for private projects or private network. Because it is not aware of credentials, network infrastructure or it handles nested URL like GitLab groups and subgroups badly. Using direct value or more proper proxy resolves that.

It is almost the same case like cyclic import issue for modules in Go. To solve that you must:

  • In Go way, put all tests in the same project/repository
  • Create 3rd repository, that will use toml and toml-test projects and cut-off any relationship between toml-test and toml

arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- This seems to work fine with Go modules too, however, when using
  GOPROXY=direct it *doesn't* work. It's not entirely clear to me if
  this is intentional or a defect. More details can be found in
  toml-lang/toml-test#74

As a workaround, this copies the toml-test package to internal/toml-test
here. This gets rid of the cyclic module dependency.

The biggest downside is that updating this is a bit awkward now. That's
okay for the time being and only affects me, and this doesn't need
updating all that often anyway. And since people are running in to real
problems now, it at least solves the immediate problem until such I have
time to investigate this in-depth and come up with a "real" solution.

The biggest downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag).
arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- This seems to work fine with Go modules too, however, when using
  GOPROXY=direct it *doesn't* work. It's not entirely clear to me if
  this is intentional or a defect. More details can be found in
  toml-lang/toml-test#74

As a workaround, this copies the toml-test package to internal/toml-test
here. This gets rid of the cyclic module dependency.

The biggest downside is that updating this is a bit awkward now. That's
okay for the time being and only affects me, and this doesn't need
updating all that often anyway. And since people are running in to real
problems now, it at least solves the immediate problem until such I have
time to investigate this in-depth and come up with a "real" solution.

The biggest downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag). I also had to add a build tag to runner.go, since it won't
be able to find the embed and io/fs imports on older versions of Go.
@niallnsec
Copy link

@arp242 regarding your query about this possibly being a bug in Go, I think I can perhaps shed a bit of light on that.

This sort of issue usually comes about as the result of a force push to a public module or a tag created locally which refers to a non-existent commit. Either way, the problem does not appear to be in Go but in the commit history of BurntSushi/toml. The commit 20a94d639b8e is orphaned and so not picked up correctly: BurntSushi/toml@20a94d6

The reason you are seeing inconsistent behaviour with proxies in play is because some will have cached versions which differ from the others.

In general, it is incorrect to have two modules which depend on one another. The reason for that is that when the go module resolver tries to work out what versions of the package to pull it will almost always end up recursively walking the version history of each repo in the dependency loop until it finds a version which does not depend on the others. This can result in unexpected behaviours in the output code.

I have had a similar problem with a circular dependency in two of my own packages, there are (to the best of my knowledge) three possible "fixes":

  1. Ensure that the repository history is consistent so that go can always walk back to an old enough version for it to find one which breaks the dependency chain. (This is very bad practice imo)
  2. Manually update the go.mod and go.sum of the two dependent packages. If you create two new tags on HEAD and link them in go.mod and go.sum manually before pushing either, you could in theory get a circular dependency working. This is also undesirable because its a very manual process you have to remember to address for every release otherwise you break other people projects. Also, it creates a small race condition in which any access of the package would fail.
  3. Remove the circular dependency. This is the best solution, as a circular dependency is technically invalid in go, just rarely caught because the resolver can usually walk back to a non-dependent package eventually. Personally I would probably merge this repository into the toml repo if it were mine, but removing the test case in the toml repo as you mentioned would work just as well.

arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- This seems to work fine with Go modules too, however, when using
  GOPROXY=direct it *doesn't* work. It's not entirely clear to me if
  this is intentional or a defect. More details can be found in
  toml-lang/toml-test#74

As a workaround, this copies the toml-test package to internal/toml-test
here. This gets rid of the cyclic module dependency.

The biggest downside is that updating this is a bit awkward now. That's
okay for the time being and only affects me, and this doesn't need
updating all that often anyway. And since people are running in to real
problems now, it at least solves the immediate problem until such I have
time to investigate this in-depth and come up with a "real" solution.

The biggest downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It doesn't "see" that these files are protected by a go1.16 build tag.

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag). I also had to add a build tag to the Go files in
toml-test.go, since it won't be able to find the embed and io/fs imports
on older versions of Go.
arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
  framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- This seems to work fine with Go modules too, however, when using
  GOPROXY=direct it *doesn't* work. It's not entirely clear to me if
  this is intentional or a defect. More details can be found in
  toml-lang/toml-test#74

As a workaround, this copies the toml-test package to internal/toml-test
here. This gets rid of the cyclic module dependency.

One downside is that updating this is a bit awkward now. That's okay for
the time being and only affects me, and this doesn't need updating all
that often anyway. Since people are running in to real problems now, it
at least solves the immediate problem until such I have time to
investigate this in-depth and come up with a "real" solution.

Another downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It doesn't "see" that these files are protected by a go1.16 build tag.

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag). I also had to add a build tag to the Go files in
toml-test.go, since it won't be able to find the embed and io/fs imports
on older versions of Go.
@tymonx
Copy link

tymonx commented Aug 5, 2021

Temporary workaround is to force using default Go proxy server for now:

GOPROXY=https://proxy.golang.org,direct go get github.com/BurntSushi/toml-test

But this is a ticking bomb that will explode in unknown time if Go proxy server will stop to resolve this correctly. And this issue is very nasty because it is affecting all projects directly or indirectly that depends anyway on the toml project (or toml-test) and any module versioning will not protect it from that 😞

@arp242
Copy link
Collaborator

arp242 commented Aug 5, 2021

The commit 20a94d639b8e is orphaned and so not picked up correctly: BurntSushi/toml@20a94d6

Right, I updated the tests in sync with the feature I was working on, and then later rebased the lot it never ended up in the master branch. I guess that the Go proxy still has this commit cached, which explains why it works for that, and why I didn't notice this locally either as I have that commit in my .git. Thanks for the explanation, it makes sense now.

I just made BurntSushi/toml#313 (before I saw your comment) which I believe should fix it using pretty much what you said: it just copies the lot in internal/.

Something along these lines is probably fine for the long term too, as it will avoid difficult to notice issues (although I did put GOPROXY=direct in the CI to avoid depending on the Go proxy to catch this sooner), and juggling around with different versions is not my idea of fun in the first place.

I'll just have to rewrite the commit message a bit to summarize the actual issue.

This should fix toml-test too once it's updated to use the new version of the toml library.

@arp242
Copy link
Collaborator

arp242 commented Aug 5, 2021

Aside:

Personally I would probably merge this repository into the toml repo if it were mine, but removing the test case in the toml repo as you mentioned would work just as well.

Not sure if that's really an option as such; the idea of this repository is that it's a general CLI that works for other (non-Go) projects as well. Actually, the idea is that it will be moved/copied to the toml organisation as the official "integration test cases", but the only person who has the permissions to do that seems to be rather inactive at the moment 😅

It's all a bit messy, but it only really affects me as the maintainer of both. I could also go back to just using the toml-test CLI, but the downside of that is that you won't be able to run tests with just go test ./..., and that you lose code coverage which is useful sometimes to see which edge cases and such have a test case and which don't.

arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
  framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- For v0.4.0 the dependencies aren't quite "correct"; it depends on
  toml-test facb9eccd4da, which in turn depends on toml 20a94d6.
  This isn't *necessarily* a problem as such as this can be resolved,
  but commit 20a94d6 on toml is not referenced at all and orphaned:
  I updated the tests in sync with the feature I was working on, and
  then later rebased the lot it never ended up in the master branch

  All of this happens to work when using the standard proxy.golang.org
  GOPROXY because it has that commit in the cache, but right not it
  doesn't work with GOPROXY=direct as that doesn't have any cache (and a
  private GOPROXY won't either).

This should be fixed by just tagging the current master as v0.4.1, as
that now refers to toml-test v1.0.0, which refers to toml v0.4.0. I'm
not super-happpy with that solution as such, because I can see it break
in the future.

So instead, just copy the toml-test package to internal/toml-test here
to get rid of any cyclic module dependency.

More details can be found in toml-lang/toml-test#74

One downside is that updating this is a bit awkward now. That's okay for
the time being and only affects me, and this doesn't need updating all
that often anyway.

Another downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It doesn't "see" that these files are protected by a go1.16 build tag.

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag). I also had to add a build tag to the Go files in
toml-test.go, since it won't be able to find the embed and io/fs imports
on older versions of Go.

This also adds GOPROXY=direct in the CI. There aren't any dependencies
in go.mod now, but this avoids depending on the peculiarities of
proxy.golang.org, and is probably a good idea for most Go CIs.
arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
  framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- For v0.4.0 the dependencies aren't quite "correct"; it depends on
  toml-test facb9eccd4da, which in turn depends on toml 20a94d6.
  This isn't *necessarily* a problem as such as this can be resolved,
  but commit 20a94d6 on toml is not referenced at all and orphaned:
  I updated the tests in sync with the feature I was working on, and
  then later rebased the lot it never ended up in the master branch

  All of this happens to work when using the standard proxy.golang.org
  GOPROXY because it has that commit in the cache, but right not it
  doesn't work with GOPROXY=direct as that doesn't have any cache (and a
  private GOPROXY won't either).

This should be fixed by just tagging the current master as v0.4.1, as
that now refers to toml-test v1.0.0, which refers to toml v0.4.0. I'm
not super-happpy with that solution as such, because I can see it break
in the future.

So instead, just copy the toml-test package to internal/toml-test here
to get rid of any cyclic module dependency.

More details can be found in toml-lang/toml-test#74

One downside is that updating this is a bit awkward now. That's okay for
the time being and only affects me, and this doesn't need updating all
that often anyway.

Another downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It doesn't "see" that these files are protected by a go1.16 build tag.

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag). I also had to add a build tag to the Go files in
toml-test.go, since it won't be able to find the embed and io/fs imports
on older versions of Go.

This also adds GOPROXY=direct in the CI. There aren't any dependencies
in go.mod now, but this avoids depending on the peculiarities of
proxy.golang.org, and is probably a good idea for most Go CIs.
arp242 added a commit to BurntSushi/toml that referenced this issue Aug 5, 2021
The problem is as follows:

- BurntSushi/toml is a TOML library.

- BurntSushi/toml-test is a generic language-agnostic TOML testing
  framework that uses this toml library.

- toml-test is based on a CLI interface. This works well, but for this
  library (since it's written in Go) it's actually quite a bit more
  convenient to integrate this in "go test"; it's more flexible, easier,
  and gives us stuff like code coverage.

- This introduces a bit of a weird dependency scenario:

  1. toml depends on toml-test
  2. toml-test depends on toml
  3. Because toml-test is only referred to from the toml_test package,
     this should actually be fine (similar to how you can use the _test
     package to work around other cyclic dependency issues).

- For v0.4.0 the dependencies aren't quite "correct"; it depends on
  toml-test facb9eccd4da, which in turn depends on toml 20a94d6.
  This isn't *necessarily* a problem as such as this can be resolved,
  but commit 20a94d6 on toml is not referenced at all and orphaned:
  I updated the tests in sync with the feature I was working on, and
  then later rebased the lot it never ended up in the master branch

  All of this happens to work when using the standard proxy.golang.org
  GOPROXY because it has that commit in the cache, but right not it
  doesn't work with GOPROXY=direct as that doesn't have any cache (and a
  private GOPROXY won't either).

This should be fixed by just tagging the current master as v0.4.1, as
that now refers to toml-test v1.0.0, which refers to toml v0.4.0. I'm
not super-happpy with that solution as such, because I can see it break
in the future.

So instead, just copy the toml-test package to internal/toml-test here
to get rid of any cyclic module dependency.

More details can be found in toml-lang/toml-test#74

One downside is that updating this is a bit awkward now. That's okay for
the time being and only affects me, and this doesn't need updating all
that often anyway.

Another downside is:

	[~c/toml](master)% du -hd1 internal/
	1.7M    internal/toml-test
	12K     internal/tag
	1.7M    internal/

1.7M is kind of a lot. But then again, it's required for running the
tests, and all of it is actual test cases. They would be in *_test.go
files otherwise anyway.

The version in the go.mod is updated because without it:

	[~c/toml](dep)% go test ./...
	# github.com/BurntSushi/toml/internal/toml-test [github.com/BurntSushi/toml.test]
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	# github.com/BurntSushi/toml/internal/toml-test
	internal/toml-test/runner.go:27:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
	FAIL    github.com/BurntSushi/toml [build failed]
	?       github.com/BurntSushi/toml/cmd/toml-test-decoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/toml-test-encoder        [no test files]
	?       github.com/BurntSushi/toml/cmd/tomlv    [no test files]
	?       github.com/BurntSushi/toml/internal     [no test files]
	?       github.com/BurntSushi/toml/internal/tag [no test files]

It doesn't "see" that these files are protected by a go1.16 build tag.

It should still work for older versions of Go though, just running these
tests won't, but that was already the case (toml_test.go has a go1.16
build tag). I also had to add a build tag to the Go files in
toml-test.go, since it won't be able to find the embed and io/fs imports
on older versions of Go.

This also adds GOPROXY=direct in the CI. There aren't any dependencies
in go.mod now, but this avoids depending on the peculiarities of
proxy.golang.org, and is probably a good idea for most Go CIs.
@arp242 arp242 closed this as completed in 31c6604 Aug 5, 2021
@arp242
Copy link
Collaborator

arp242 commented Aug 5, 2021

Okay, I released v1.0.1 of this toml-test tool, and v0.4.1 of the toml library with the changes outlined above.

I believe this should now be fixed:

[~]% go clean -modcache
[~]% export GOPROXY=direct
[~]% go get -v -u github.com/BurntSushi/toml-test
go: downloading github.com/BurntSushi/toml-test v1.0.1
get "zgo.at/zli": found meta tag vcs.metaImport{Prefix:"zgo.at/zli", VCS:"git", RepoRoot:"https://github.com/zgoat/zli.git"} at //zgo.at/zli?go-get=1
go: downloading github.com/BurntSushi/toml v0.4.1
github.com/BurntSushi/toml/internal
github.com/BurntSushi/toml
github.com/BurntSushi/toml-test

[~]% go clean -modcache
[~]% go get -v -u github.com/BurntSushi/toml
go: downloading github.com/BurntSushi/toml v0.4.1

Thanks again Tymoteusz and Niall for your help!

@tymonx
Copy link

tymonx commented Aug 5, 2021

@arp242 Great! I can confirm that. It works perfectly now 👍

@niallnsec
Copy link

Thanks for the quick turnaround @arp242 👍

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

No branches or pull requests

4 participants