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

replace bingo with go 1.17 go run commands #314

Closed
codefromthecrypt opened this issue Jul 22, 2021 · 7 comments · Fixed by #350
Closed

replace bingo with go 1.17 go run commands #314

codefromthecrypt opened this issue Jul 22, 2021 · 7 comments · Fixed by #350
Labels
enhancement New feature or request

Comments

@codefromthecrypt
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Right now, we use a modified version of bingo to pin versions without tainting GOBIN's default. For example, this is used to invoke known versions of hugo, goreleaser, golint, etc

Describe the solution you'd like
Inside the Makefile, declare paths for each main tool and do go run instead. This reduces the toolchain knowledge to run the project and conversion to make-unfriendly env like windows more straightforward.

go run now accepts arguments with version suffixes (for example, go run example.com/cmd@v1.0.0). This causes go run to build and run packages in module-aware mode, ignoring the go.mod file in the current directory or any parent directory, if there is one. This is useful for running executables without installing them or without changing dependencies of the current module.

Describe alternatives you've considered
Leaving it alone. There is some benefit to leaving it as depending on how go run in 1.17 is implemented, it might be inefficient to repeatedly run the same command.

Additional context
https://tip.golang.org/doc/go1.17#go-command

@codefromthecrypt codefromthecrypt added the enhancement New feature or request label Jul 22, 2021
@codefromthecrypt
Copy link
Contributor Author

cc @nacx

@mathetake
Copy link
Member

this is good to know this, thanks for raising here 😄

@codefromthecrypt
Copy link
Contributor Author

It is possible that when we use this, we can avoid needing cygwin bash anymore. So, possibly worth revisiting the github action that currently installs cygwin-bash and overrides the shell in order to translate paths so bingo's Variables.mk doesn't die (#325)

codefromthecrypt pushed a commit that referenced this issue Aug 24, 2021
This uses versioned syntax of `go run` brought to us in 1.17
https://golang.org/doc/go1.17

In doing so, we can remove the complexity of bingo and also simplify its
impact on windows.

Fixes #314

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this issue Aug 24, 2021
This uses versioned syntax of `go run` brought to us in 1.17
https://golang.org/doc/go1.17

In doing so, we can remove the complexity of bingo and also simplify its
impact on windows.

Fixes #314

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>
@nacx
Copy link
Member

nacx commented Sep 8, 2021

When I was trying to upgrade the monorepo to 1.17 and move away from Bingo, I found this error when trying to use Kustomize:

go generate ./...
+ set -o allexport
+ source ../../../../scripts/build/tools.mk
++ GOIMPORTS=golang.org/x/tools/cmd/goimports@v0.1.5
++ GOLANGCI_LINT=github.com/golangci/golangci-lint/cmd/golangci-lint@v1.41.1
++ KUSTOMIZE=sigs.k8s.io/kustomize/kustomize/v3@v3.10.0
++ LICENSER=github.com/liamawhite/licenser@v0.6.1-0.20210729145742-be6c77bf6a1f
+ set +o allexport
+ go run sigs.k8s.io/kustomize/kustomize/v3@v3.10.0 build sources/managementplane -o managementplaneoperator.yaml
go run: sigs.k8s.io/kustomize/kustomize/v3@v3.10.0 (in sigs.k8s.io/kustomize/kustomize/v3@v3.10.0):
	The go.mod file for the module providing named packages contains one or
	more exclude directives. It must not contain directives that would cause
	it to be interpreted differently than if it were the main module.

Looks like go run does not run if the referenced modules contain replace or exclude directives. I found some of the rationale here: golang/go#40276.

Have you found similar issues? And if so, do you have a workaround? Given how messy the dependency hell problem is in all languages, we could expect exlude/replace directives to be not a rare thing.

@codefromthecrypt
Copy link
Contributor Author

well we don't have need for excludes since axing the istio deps :D

@codefromthecrypt
Copy link
Contributor Author

serious answer: 😎 personally, I would try to push that onto the tools themselves, ex keep a bingo file around for things not supported, yet, or just do the same thing ad-hoc in your make file. Raise an issue with the repo that has the conflicts and at least inform them that their tool doesn't work because they are needing to do replaces and whatnot. Some libraries have alternatives that don't have dependency hell, and in any case you can help build pressure on the original go issue that was denied, and through all of it have paperwork in case things change (ex the links to the issues that might resolve)

@nacx
Copy link
Member

nacx commented Sep 10, 2021

Looks like it already was reported. Let's see! kubernetes-sigs/kustomize#3618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants