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

Put the github.com/BurntSushi/toml-test in internal/ #313

Merged
merged 1 commit into from Aug 5, 2021
Merged

Put the github.com/BurntSushi/toml-test in internal/ #313

merged 1 commit into from Aug 5, 2021

Conversation

arp242
Copy link
Collaborator

@arp242 arp242 commented 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.

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.
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.

None yet

1 participant