-
Notifications
You must be signed in to change notification settings - Fork 852
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
main: support comma-delimited -tags #4088
base: dev
Are you sure you want to change the base?
Conversation
Converted to draft, I'll also update loader/list.go noted in the issue ticket. |
8e99a74
to
e6e4835
Compare
I've commented on golang/go#44787, let's see whether we get a response in a reasonable timeframe. |
@omnide can you try to get this fixed upstream? See golang/go#44787 (comment) for details. While we can work around this in TinyGo, the proper place to fix this would be upstream. So if at all possible, I'd prefer that over rolling our own solution. |
Hey @aykevl, I totally agree about upstream fix being preferred here. I'll follow up on golang/go#44787 and the linked gerrit. |
I opened https://go-review.googlesource.com/c/tools/+/558115 and will keep it moving until we have a resolution. I now have a signed CLA for golang and can contribute there. |
9973a87
to
3d68f8b
Compare
Updates to golang tools finally added support for comma-delimited tags in v0.18.0 released mid Feb 2024. Updating the dependency brings this ability to tinygo as well since it uses buildutil.TagsFlag See upstream fix at: golang/tools@5f90691 Also add GOTEST override in makefile to permit use of gotestsum when running tests interactively: GOTEST="gotestsum --" GOTESTFLAGS=-v make test
Updated: the upstream fix I made to golang/tools was released in v0.18.0 so this PR has been updated to just use the latest release and pick that up. I updated instances where build tags were still using spaces in the makefile, retained the change for loader/list.go. This also includes a makefile update to allow override of the
go test
command so that utilities like gotestsum can be used:GOTEST="gotestsum --" make test
.Base go tooling originally supported tags as spaced separated list, but more recent versions use comma-separated. The old space delimited form is still supported but deprecated per the latest usage dumps.
In earlier golang discussion around the use of commas, the fixes just added error messages if commas were found, but this was later changed.
golang/go#18800
The current version in
go build
permits the old style (< go1.13) with spaces instead of commas, but does not allow mixing the two forms. So either "tagA tagB tagC" or "tagA,tagB,tagC", but NOT "tagA,tagB tagC". Need to check with tinygo maintainers if they have a preference here.Blame from go lang cmd internals:
https://go.googlesource.com/go/+/80e7832733fd245181af3394077f2df21303a4aa%5E%21/src/cmd/go/internal/work/build.go
Here Russ Cox favors commas and deprecates spaces.
More recent issue related to buildutil.TagsFlag, which only handles the spaced form. Issue remains open so we can't rely on TagsFlag to manage this for us.
golang/go#44787
Golang contributor opened a PR to address it but it stalled in 2021. A golang maintainer requested additional test coverage, which was provided but then no reply after that.
https://go-review.googlesource.com/c/tools/+/284214#message-fc9907b03ae75ae24145f421e623330b4b9b9158
The associated github PR notes that they couldn't locate a CLA for the committer identity golang tools pr 268. I also do not have a CLA for this.
I think it would be best to accept either form, even combinations. But also amenable to stricter validation and just exiting with a message about not mixing forms.
Example with flexible fix in place:
Closes #3966.