covidatlas / li

Next-generation serverless crawler for COVID-19 data
Apache License 2.0
57 stars 33 forks source link

Fix build.yml so we actually run on Windows and macOS #590

Closed joliss closed 3 years ago

joliss commented 3 years ago

Right now we just run on ubuntu-latest three times.

See for example https://github.com/covidatlas/li/commit/d06f2ea5fb0a6647ef42b818991d57e6cf29bcb9/checks#step:1:3, which has os = windows-latest, but is actually running on Ubuntu.

I'm assuming this PR will uncover some tests that'll fail on Windows or Mac, so perhaps we should hold off on merging it until those are fixed.

joliss commented 3 years ago

So it looks like there's a whole bunch of things going wrong on Windows; see the test output on Windows. I don't think this is something I want to try and fix, especially since I don't have a Windows development box set up.

Do we have any volunteers to try and fix this? Or do we just give up on the Windows build for now?

ryanblock commented 3 years ago

Wow, huge find, thank you!

ryanblock commented 3 years ago

One thing that occurs to me looking at this: npm config set script-shell "C:\\Program Files\\Git\\bin\\bash.exe" – may want to drop that in favor of just using cross-env, wdyt?

Update: on second thought, prob not, idk!

jzohrab commented 3 years ago

Let's give up on the Windows build for now. We don't have any devs on windows, prod isn't windows, and if we do get one who has dev experience, then that can be their first concrete task to work on. I'm running on mac with no issues during local test and dev, which is sufficient for me.

Great find, @joliss . I've created a fix-windows-ci branch using your work, and opened https://github.com/covidatlas/li/issues/591. I'll check macOS in a separate branch, and will update master.

jzohrab commented 3 years ago

Verifying macOS ci works in a separate branch -- run details are at https://github.com/covidatlas/li/runs/1008043577?check_suite_focus=true.

Opening a new PR which will focus on what works. Thanks @joliss ! jz

jzohrab commented 3 years ago

Closing in favor of #592.