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

Add 'unix' condition #169

Merged
merged 2 commits into from
Jul 28, 2022
Merged

Add 'unix' condition #169

merged 2 commits into from
Jul 28, 2022

Conversation

bitfield
Copy link
Contributor

(see commit messages)

Fixes #166.

Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Change itself SGTM - I didn't notice that the goos and goarch conditions weren't documented yet.

As for the test, I would say use a "nested" testscript test. See the testdata changes in https://github.com/rogpeppe/go-internal/pull/167/files, for example. You could check that [unix] matches when the host is [linux] and not when it's [windows], for example - that should keep things simple enough and get us covered with CI.

@bitfield
Copy link
Contributor Author

Sorry, I'm not completely clear on what you're suggesting. Could you elaborate a little?

You could check that [unix] matches when the host is [linux] and not when it's [windows]

That was the aim of the two tests guarded by mutually exclusive build tags—I couldn't find a way to get testscript to "pretend" to be running on a particular GOOS. Setting env GOOS=linux in the script has no effect. Is there a neater way to do this?

@mvdan
Copy link
Collaborator

mvdan commented Jul 27, 2022

Just pushed a commit to show what I mean :) I ended up deciding against using nested testscript, because we really don't need it. The end result is very close to what you had done, but with far less test code, thanks to testscript itself. The test only checks linux/darwin/windows, but I reckon that is enough given how popular they are, and that they are the three OSes that we use CI on.

@mvdan
Copy link
Collaborator

mvdan commented Jul 27, 2022

Oh, and while there, I also rewrote the existing cond.txt to use the native mkdir and exists rather than an external shell, which means more portability - particularly necessary for Windows. So perhaps we should wait for an extra review from @myitcv or @rogpeppe since I can't really approve my own changes there.

@bitfield
Copy link
Contributor Author

Great! Very good suggestions, thank you @mvdan.

@mvdan
Copy link
Collaborator

mvdan commented Jul 27, 2022

If it sounds good to you, then squash into your second commit - then we're ready for another review and a merge.

Copy link
Collaborator

@myitcv myitcv left a comment

Choose a reason for hiding this comment

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

LGTM with one question.

imports/build.go Show resolved Hide resolved
bitfield and others added 2 commits July 28, 2022 10:04
From Go 1.19, the build constraint 'unix' proposed in golang/go#20322 is
satisfied by any sufficiently Unix-like value of GOOS, as defined by
src/go/build/syslist.go. This commit adds a 'UnixOS' list containing the
values of GOOS that would satisfy the 'unix' constraint in Go 1.19.

Co-authored-by: Paul Jolly <paul@myitcv.io>
From Go 1.19, the build constraint 'unix' proposed in golang/go#20322 is
satisfied by any sufficiently Unix-like value of GOOS, as defined by
src/go/build/syslist.go. This commit adds a predefined 'unix' condition
with the same meaning, available for use in test scripts. The condition
is satisfied if the target GOOS is one of the list of Unix-like systems
defined in 'imports.UnixOS'.

Fixes rogpeppe#166.

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>
@mvdan mvdan merged commit 2431384 into rogpeppe:master Jul 28, 2022
@bitfield bitfield deleted the add-cond-unix branch July 28, 2022 14:45
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.

testscript: add 'unix' condition?
3 participants