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

Brings back windows testing to CI #685

Merged
merged 44 commits into from
Jul 24, 2020
Merged

Conversation

euri10
Copy link
Member

@euri10 euri10 commented May 28, 2020

This adds the testing on a windows machine that was lost during travis > github actions
Given the recent number of it seems windows only issues it might be a good 1st step
I dont have windows so it's always hard to pinpoint what's going on sometimes !

@euri10 euri10 requested a review from a team May 28, 2020 06:32
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Fab, yup this'd be a great step.

Is there any reason we can't use the following style?...

      - name: "Install dependencies"
        run: "scripts/install"
      - name: "Run tests"
        run: "scripts/test"

If there's any platform specific stuff in either of those scripts we can deal with that inside the scripts themselves.

@euri10
Copy link
Member Author

euri10 commented May 28, 2020 via email

@euri10
Copy link
Member Author

euri10 commented May 28, 2020

just googled it a bit
actions/starter-workflows#154 (comment)
still this requires a ps1 or a specific windows script, will see if I find something more interesting

@tiangolo
Copy link
Sponsor Member

I was just checking, it seems like maybe it would be possible to use Bash on Windows in GitHub Actions... 🤔 ?

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#using-a-specific-shell

The default shell on non-Windows platforms with a fallback to sh. When specifying a bash shell on Windows, the bash shell included with Git for Windows is used.

I would also expect it to require PowerShell, but if it's possible to keep just Bash scripts I think that would be great 🚀

@euri10
Copy link
Member Author

euri10 commented Jul 20, 2020

seems to be cool now, could I get a review @tomchristie or @tiangolo ? 🥼

I had to modify a little the install script to detect the os so that it doesnt install uvloop under windows, for that I changed the sh to bash as OSTYPE is not available under the original bourne shell :)

that said if #666 is merged the requirements_windows.txt can be safely removed and that part of the script would become useless

@euri10 euri10 requested a review from tomchristie July 20, 2020 17:41
scripts/install Outdated Show resolved Hide resolved
scripts/install Outdated
#!/bin/sh -e
#!/usr/bin/env bash

set euxo -pipefail
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using it in all my bash scripts since I read this a few years ago (https://vaneyckt.io/posts/safer_bash_scripts_with_set_euxo_pipefail)

can remove it if you want

Copy link
Member Author

Choose a reason for hiding this comment

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

e exits when a command fails
o pipefail set the exit code to the righmost cmd to exit with a non 0 status
with u you're not allowed to use unset variables
x is useful for debug, it print the commands befre executing them

Copy link
Member Author

Choose a reason for hiding this comment

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

and you're right there was a typo it's -euxo pipefail
https://explainshell.com/explain?cmd=set+-euxo+pipefail

scripts/install Outdated Show resolved Hide resolved
scripts/install Outdated
#!/bin/sh -e
#!/usr/bin/env bash

set -exo pipefail
Copy link
Member

Choose a reason for hiding this comment

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

I reckon we should probably omit this. The pipefail is extraneous here, and would have already included -x if had wanted that.

We could potentially use set -x immediately prior running to the pip install commands, but let's treat that all as a separate PR and try to keep this as absolutely isolated as possible to "what do we need to do in order to support running the tests on windows".

@tomchristie
Copy link
Member

Looks great, yup, so approved.
Not sure what's up with the status checks here tho - might be something glitchy that resolves once we bring this in, perhaps?

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