Brings back windows testing to CI #685
Conversation
There was a problem hiding this comment.
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.
|
I may be totally off because I dont know windows well, but I see 2 issues,
1) this is windows so sh scripts should be adapted to powershell I bet, 2)
the set of requirements is different between both which the install script
doesnt deal with,
I began just changing the machine name and the CI passed but dod nothing on
install and test scripts
so one think we could do is clone the sh script into powershell, put a
requirements.windows.txt and run that.
or maybe there's a way to run sh scripts in windows, I dont know
…On Thu, May 28, 2020 at 3:09 PM Tom Christie ***@***.***> wrote:
***@***.**** commented on this pull request.
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.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#685 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAINSPS43BOOSZSEAYN7N4TRTZPCFANCNFSM4NMZFQCQ>
.
--
benoit barthelet
http://pgp.mit.edu/pks/lookup?op=get&search=0xF150E01A72F6D2EE
|
|
just googled it a bit |
|
I was just checking, it seems like maybe it would be possible to use Bash on Windows in GitHub Actions... 🤔 ?
I would also expect it to require PowerShell, but if it's possible to keep just Bash scripts I think that would be great 🚀 |
|
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 |
scripts/install
Outdated
| #!/bin/sh -e | ||
| #!/usr/bin/env bash | ||
|
|
||
| set euxo -pipefail |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
and you're right there was a typo it's -euxo pipefail
https://explainshell.com/explain?cmd=set+-euxo+pipefail
scripts/install
Outdated
| #!/bin/sh -e | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -exo pipefail |
There was a problem hiding this comment.
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".
|
Looks great, yup, so approved. |
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 !