Improve on "Add support for Windows builds" #52
Conversation
(try to bridge the gap with gleam-lang/setup-erlang that seems to support pre-21 versions)
|
I split the CI files into three different files. Rationale: easier to see where an error comes from, easier to have more inspectable job names. |
| @@ -9,12 +9,8 @@ FILE_INPUT="${VSN}.zip" | |||
| FILE_OUTPUT=elixir.zip | |||
| DIR_FOR_BIN=.setup-beam/elixir | |||
|
|
|||
| rm -f "${FILE_OUTPUT}" | |||
There was a problem hiding this comment.
Removed what was unnecessary, even if old code. It makes for easier maintenance.
| $ProgressPreference="SilentlyContinue" | ||
| Invoke-WebRequest "https://github.com/erlang/otp/releases/download/OTP-$VSN/otp_win64_$VSN.exe" -OutFile "$FILE_OUTPUT" | ||
| $ProgressPreference="Continue" | ||
| New-Item "$DIR_FOR_BIN" -ItemType Directory | Out-Null | ||
| Move-Item "$FILE_OUTPUT" "$DIR_FOR_BIN" | ||
| Start-Process "./$DIR_FOR_BIN/$FILE_OUTPUT" /S -Wait | ||
| Write-Output "C:/Program Files/erl-$VSN/bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append | ||
| $ErlExec = Get-ChildItem -Path "C:/Program Files/" -Recurse -Depth 2 -Filter 'erl.exe' -Name | ForEach-Object { Write-Output "C:/Program Files/$_" } |
There was a problem hiding this comment.
Yeah, so the new approach is to find erl.exe and make the container folder part of the path. 🎉 , more Windows versions supported.
| @@ -13,7 +13,7 @@ async function installOTP(osVersion, otpVersion) { | |||
| await exec(path.join(__dirname, 'install-otp.sh'), [osVersion, otpVersion]) | |||
| } else if (OS === 'win32') { | |||
| const script = path.join(__dirname, 'install-otp.ps1') | |||
| await exec(`powershell.exe ${script} -VSN:${otpVersion}`) | |||
There was a problem hiding this comment.
I got into trouble with powershell and paths. This removed my obstacles.
| @@ -17,19 +18,15 @@ async function main() { | |||
| const otpVersion = await installOTP(otpSpec, osVersion) | |||
|
|
|||
| const elixirSpec = core.getInput('elixir-version', { required: false }) | |||
| const shouldMixHex = core.getInput('install-hex', { | |||
There was a problem hiding this comment.
This is actually just moving code around, to revert a previous decision that created more confusion that expected.
| @@ -342,14 +337,6 @@ async function get(url0, pageIdxs) { | |||
| return ret | |||
| } | |||
|
|
|||
| function prependToPath(what) { | |||
There was a problem hiding this comment.
I learned, yesterday, that .addPath existed. It's a nice replacement for this.
|
@starbelly, I'm going to ask for your review here. There's nothing special in the pull request but some extra eyes are always nice. You could probably also run this over that project you have with multiple rebar3 versions and whatnot, even using Windows, maybe (?) |
|
Tests are failing because 22.3 is getting converted to 22.3.0 (ranges, again!) which doesn't exist; we might want to tackle this with #52 soon (potentially even merge that into this pull request). |
src/setup-beam.js
Outdated
| if (versions.includes(spec)) { | ||
| if ( | ||
| versions.includes(spec) || | ||
| core.getInput('version-type', { required: false }) === 'strict' |
There was a problem hiding this comment.
I'm tentatively adding this here. I ran into version-related trouble while implementing Windows and feel since this has been reported as an issue before it might just be time to implement it. Adding it to this pull request since a bug, made evident, seems impossible to overcome with such a mechanism.
|
I believe there might be some more |
|
I'm pushing an update to the way we find non-semver stuff in the versions we have. I've recently introduced the |
| @@ -16,6 +16,8 @@ async function all() { | |||
| await testOTPVersions() | |||
| await testElixirVersions() | |||
| await testRebar3Versions() | |||
|
|
|||
| await testGetVersionFromSpec() | |||
There was a problem hiding this comment.
Mind you none of the other tests change, so this should give us some confidence in backward compatibility.
There was a problem hiding this comment.
Well, some actually change, but the update only made the previous implementation issue (not getting the latest latest) into evidence, so I guess this is good.
| @@ -278,11 +266,24 @@ async function getRebar3Versions() { | |||
| } | |||
|
|
|||
| function getVersionFromSpec(spec, versions) { | |||
| if (versions.includes(spec)) { | |||
| return spec | |||
| if (core.getInput('version-type', { required: false }) === 'strict') { | |||
There was a problem hiding this comment.
The new option (version-type)...
src/setup-beam.js
Outdated
| } | ||
|
|
||
| return semver.maxSatisfying(versions, semver.validRange(spec)) | ||
| // We keep a map of semver => actualver in order to use semver ranges to find appropriate versions |
There was a problem hiding this comment.
... and the new way we keep references in a map for further handling. Tests should be the stuff giving us confidence over this, so we should improve/add to those if you feel it's warranted.
(put in evidence that some stuff was wrong, in the past)
| return version | ||
| } | ||
|
|
||
| function sortVersions(left, right) { |
There was a problem hiding this comment.
This one's a proper attempt at solving OTP-like versions, with 5 elements like 0.0.0.0.0
| @@ -224,10 +219,7 @@ async function getOTPVersions(osVersion) { | |||
| .filter((x) => x.name.match(/^otp_win64_.*.exe$/)) | |||
| .forEach((x) => { | |||
| const otpMatch = x.name.match(/^otp_win64_(.*).exe$/) | |||
| let otpVersion = otpMatch[1] | |||
There was a problem hiding this comment.
This was moved to getVersionFromSpec to centralise version management/handling.
| @@ -209,11 +208,7 @@ async function getOTPVersions(osVersion) { | |||
| .split('\n') | |||
| .forEach((line) => { | |||
| const otpMatch = line.match(/^(OTP-)?([^ ]+)/) | |||
|
|
|||
There was a problem hiding this comment.
This was moved to getVersionFromSpec to centralise version management/handling.
|
This is ready for general review. |
| @@ -0,0 +1,87 @@ | |||
| --- | |||
There was a problem hiding this comment.
I like splitting these out into different workflows for easy digestion 👍
| @@ -192,6 +196,149 @@ async function testRebar3Versions() { | |||
| assert.deepStrictEqual(got, expected) | |||
| } | |||
|
|
|||
| async function testGetVersionFromSpec() { | |||
There was a problem hiding this comment.
Definitely not something to do here, but an interesting idea might be to property test functions like these. There are indeed js libs for this. Might be overkill, but makes sense to me 🤷
|
Stellar work as usual @paulo-ferraz-oliveira ❤️💛💚💙💜 |
|
I'm going to test this in some repo.s that use Windows and then, based on those results, will either merge or perform smaller fixes. |
|
This is working here:
|
|
@starbelly, based on the previous message and your approval, could you unblock the merge so I can squash + merge? Edit - maybe this'll help: https://github.community/t/expected-waiting-for-status-to-be-reported/18001. |
|
I'm going to try to close and reopen this to see if the workflow issue goes away. Edit: it doesn't 😢. |
|
It's merged. Thanks for all the help, Bryan. |
| default: true | ||
| rebar3-version: | ||
| description: Version range or exact version of rebar3 to use | ||
| version-type: |
There was a problem hiding this comment.
just wanted to say a big thank you for this option in particular (and the whole project in general.) :D
Closes #51. Also closes #50. Also closes #53. Also closes #46.
ci.ymlwithtest.yml(and tweak output text) - tweaking the titles is not particularly helpful (GitHub still adds the matrix variables to it, so it makes for a less readable output)