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

Fix windows support in www/install.sh #1474

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

bloodearnest
Copy link
Contributor

The current version of install.sh does not quite fully support installing windows binaries.

This change fixes that by:

  • handling the fact that windows releases are zips not tarballs
  • handling uname -s output being funky on MINGW compiled bash

Additionally, it adds a step to the CI tests to test install.sh works on multiple platforms. These tests uncovered that need rev was failing on windows bash. However, rev is not actually used any more, so I removed that and need basename, as that doesn't seem to be used any more either.

The current version of install.sh does not quite fully support
installing windows binaries.

This change fixes that by:

 - handling the fact that windows releases are zips not tarballs
 - handling `uname -s` output being funky on MINGW compiled bash

Additionally, it adds a step to the CI tests to test install.sh works on
multiple platforms. These tests uncovered that `need rev` was failing on
windows bash.  However, `rev` is not actually used any more, so
I removed that and `need basename`, as that doesn't seem to be used any
more either.
bloodearnest added a commit to opensafely-core/setup-action that referenced this pull request Jan 4, 2023
This follows the official just instructions for [installing pre-built
binaries](https://github.com/casey/just#pre-built-binaries).

However, the current version of https://just.systems/install.sh doesn't
quite work in windows. I have fixed that and [proposed the changes
upstream](casey/just#1474), and include a copy
of the fixed script in this repo for now.

As part of this, have added tests for this action, which runs on
multiple OSs, in order to test the cross platfoem installation

Also, allow installing a specific version of just.
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@casey
Copy link
Owner

casey commented Jan 4, 2023

This is a great PR, thank you! I think Windows support in install.sh has been broken for a long time. (Possibly forever, and nobody actually tried it / noticed / reported it 😅)

@casey casey merged commit a1ac579 into casey:master Jan 4, 2023
@bloodearnest
Copy link
Contributor Author

bloodearnest commented Jan 4, 2023

Thanks for your attention, and the merge 🎉

For context, in case it's of interest, the reason behind this fix is we were using https://github.com/extractions/setup-just to install just in our GH actions.

However, under the hood, this action uses the Github API to get the current release. Due to Github runners sharing IPs, we would relatively frequently get rate limit errors in our CI. The documented workaround is pass our GITHUB_TOKEN as a parameter, but we're not keen to send that to a third party action.

So, we ended up moving to using our own action, based on https://just.systems/install.sh to install the release (as it uses curl rather than GH API), thus avoiding the rate limits. However, we have some projects that need Windows CI, hence this PR.

Thanks for just, its great!

@bloodearnest bloodearnest deleted the fix-install-sh-windows branch January 4, 2023 22:25
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.

2 participants