search

asdf-community / asdf-golang

Go plugin for the asdf version manager
https://github.com/asdf-vm/asdf
MIT License
552 stars 92 forks source link

go.mod is not an asdf-compatible version file #99

Closed rliebz closed 1 year ago

rliebz commented 1 year ago

Describe the bug The go directive in the go.mod file does not indicate the Go executable version that should be used to build the project. In particular, the go tool explicitly works with versions other than the version in the go directive. Much of the language in the Go module reference suggests this to be the case:

A go directive indicates that a module was written assuming the semantics of a given version of Go.

For packages within the module, the compiler rejects use of language features introduced after the version specified by the go directive.

If an older Go version builds one of the module’s packages and encounters a compile error, the error notes that the module was written for a newer Go version.

Additionally, the go command changes its behavior based on the version specified by the go directive.

More evidence that the go directive is not prescriptive about the specific Go version that it should be built with is evidenced by a discussion about the possibility of incorporating a toolchain version into the go.mod file and go tool, independently from the existing directive: https://github.com/golang/go/discussions/55092

Additionally, this plugin's treatment of go.mod version selection explicitly conflicts with the rules provided by asdf:

Note that this script should be deterministic and always return the same exact version when parsing the same legacy file. The script should return the same version regardless of what is installed on the machine or whether the legacy version is valid or complete. Some legacy file formats may not be suitable.

This explicitly violates that spec rule by looking at the installed versions, and having no deterministic version resolution (e.g., what version should be used if go 1.20 is specified).

Because asdf does not allow enabling/disabling of specific legacy file formats on a per-tool basis, it is not possible to opt into the standard asdf treatment of legacy files without also opting into this plugin's non-standard interpretation of go.mod and the asdf functionality.

To Reproduce

  1. Enable legacy_version_file = yes
  2. Set a version of go with asdf global golang system (or other)
  3. Open up a Go project with a go.mod file and no .tool-versions
  4. The global asdf/golang version is not respected, because go.mod is incorrectly treated as a version file

Expected behavior The global golang version will be chosen

Actual behavior The go version is chosen non-deterministically based on the local go.mod file

Additional context Broadly speaking, there are three ways to address this:

  1. Remove the current behavior completely. This is my recommendation, as it keeps the plugin simple and conforms to standard go.mod and asdf spec interpretation
  2. Make the current behavior opt-in, such as through use of an an environment variable (e.g., export ASDF_GOLANG_MOD_VERSION=true)
  3. Make the current behavior opt-out, such as through use of an an environment variable (e.g., export ASDF_GOLANG_MOD_VERSION_DISABLED=true). I would personally not recommend this, because it requires configuration to prevent non-standard behavior

I'm happy to make the contribution, but I wanted to get buy in before starting work.

kennyp commented 1 year ago

If I were doing it all over again, I probably would go with option 1 and not include the behavior to begin with, but the guidance from asdf around legacy versions came much later. At this point I'm hesitant to break the current behavior outright, but perhaps we can deprecate it over time.

What do you think about a hybrid approach. If the user has neither ASDF_GOLANG_MOD_VERSION_{ENABLED,DISABLED} set, we continue the current behavior, but emit a warning to stderr that in the future users will need to set one or the other. Then in the future we move to requiring ASDF_GOLANG_MOD_VERSION_ENABLED?

kennyp commented 1 year ago

@rliebz thoughts on #101?

rliebz commented 1 year ago

Yeah that seems like a great compromise between backward compatibility for now and asdf standardization long-term. Thanks for taking the time to respond and put that change together.

👍 from me!

kennyp commented 1 year ago

I'm going to go ahead and mark this as closed for now as folks can opt in/out after merging #101. I'll formulate a timeline for switching the default to false... probably when it comes up again. :wink:

MPV commented 1 year ago

I might be wrong, but I'm not sure if the changes in #101 worked as intended (i.e. can't be disabled), see this issue: