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

testscript: add build:tag condition #150

Closed
wants to merge 2 commits into from

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Dec 15, 2021

As discussed with @mvdan in #149.

@twpayne
Copy link
Contributor Author

twpayne commented Dec 15, 2021

The build: prefix is needed to distinguish between set and not-set tags. If the syntax is only tag (e.g. [go1.19]) then it is not possible to distinguish between a not-set tag and an unknown condition.

Copy link
Owner

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow review. This LGTM modulo a couple of minor suggestions, and I think it supercedes #149, right? Thanks for proposing it!

testscript/doc.go Outdated Show resolved Hide resolved
testscript/testdata/build.txt Outdated Show resolved Hide resolved
@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2022

Thanks for the review and zero stress about the review time - you're giving the gift of open source so there are no expectations or obligations.

I've applied your suggested changes and rebased on the latest master, so this should be good to merge.

@twpayne
Copy link
Contributor Author

twpayne commented Jan 31, 2022

Anything else needed here?

@mvdan
Copy link
Collaborator

mvdan commented Mar 1, 2022

Apologies for my delay here :) Two thoughts:

  1. I still think that we should just move this part of gotooltest into the main testscript package, so that conditions like [go1.17] or [gc] are supported by all test scripts. Like you say, this can be useful for any program written in Go, even if it doesn't interact with the Go toolchain and doesn't need gotooltest. Supporting both [go1.17] and [build:go1.17] seems unnecessary.

  2. Similar to the above, I don't really agree with adding [build:buildtag] support when we in fact don't support arbitrary build tags. At best, it's unnecessary, and we should stick with [go1.17] as per the above. But I'm also worried about misleading users into thinking that e.g. [build:purego] will work when it won't.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 2, 2022

#154 implements @mvdan's suggestions, superseding this PR.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 2, 2022

Closing in favor of #154.

@twpayne twpayne closed this Mar 2, 2022
@twpayne twpayne deleted the build-condition branch March 2, 2022 16:52
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

3 participants