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

main: support comma-delimited -tags #4088

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

omnide
Copy link

@omnide omnide commented Jan 16, 2024

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:

for t in "comma,delim" "space delim" "comma,and space delim"; do tinygo info -tags ${t} | grep "build tags"; done
build tags:        linux amd64 tinygo math_big_pure_go gc.precise scheduler.tasks serial.none comma delim
build tags:        linux amd64 tinygo math_big_pure_go gc.precise scheduler.tasks serial.none space delim
build tags:        linux amd64 tinygo math_big_pure_go gc.precise scheduler.tasks serial.none comma and space delim

Closes #3966.

@omnide omnide marked this pull request as draft January 16, 2024 13:56
@omnide
Copy link
Author

omnide commented Jan 16, 2024

Converted to draft, I'll also update loader/list.go noted in the issue ticket.

@omnide omnide marked this pull request as ready for review January 16, 2024 22:45
@aykevl
Copy link
Member

aykevl commented Jan 19, 2024

I've commented on golang/go#44787, let's see whether we get a response in a reasonable timeframe.

@aykevl
Copy link
Member

aykevl commented Jan 22, 2024

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

@omnide
Copy link
Author

omnide commented Jan 22, 2024

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

@omnide
Copy link
Author

omnide commented Jan 24, 2024

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.

@omnide omnide force-pushed the fix-tags-commas branch 3 times, most recently from 9973a87 to 3d68f8b Compare March 4, 2024 02:27
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
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

2 participants