Add support for Windows builds #49
Conversation
| @@ -0,0 +1,5 @@ | |||
| Write-Output "Installer for Elixir for Windows not available" | |||
There was a problem hiding this comment.
He/she/it who comes after is more than welcome to take a stab at this. For the time being I preferred to be explicit in saying Elixir is not available.
There was a problem hiding this comment.
It's also not clear whether the elixir web installer can do unattended installs to boot.
There was a problem hiding this comment.
I thought the main selling point of Elixir web installer was that it would install OTP for you as well. Which is not what we want as we want more control into particular OTP versions.
I think we could treat Elixir tarball similar to Erlang tarballs, we unpack it somewhere and add bin/ to path. Elixir ships with Windows scripts (https://github.com/elixir-lang/elixir/tree/master/bin). I don't know if it is relevant but it looks like it only has .ps1 for mix and not others.
THat being said, probably out of the scope of this PR.
There was a problem hiding this comment.
Ahh yeah, that makes sense. I've never installed elixir on windows, but yeah I would think the precompiled tarball should "just work" ™️
There was a problem hiding this comment.
THat being said, probably out of the scope of this PR.
If it very simple to add (ie.I have instructions or pointers) I don't mind doing it - in that sense it's not out-of-scope because Elixir was part of the mix already, but I don't have an easy way to do it, since I might miss something. With Erlang/OTP I knew exactly what I was looking for.
There was a problem hiding this comment.
That said, can any of you give me pointers as to how this could be achieved?
There was a problem hiding this comment.
There was a problem hiding this comment.
we should of course download from repo.hex.pm, same as we do for Linux, because we want to grab Elixir compiled against a matching OTP version.
in fact, do we even need install-elixir.ps1? perhaps it's easier to do everything in JS because it is supposed to work cross-platform, all we need to do is unzip and add the bin/ directory to $GITHUB_PATH. Again I'm not well versed to comment on the implementation, pick whatever is the simplest and most maintainable.
There was a problem hiding this comment.
Yeah, the other day I was wondering why some stuff was written as Shell script vs JavaScript, myself. I think it should only be so when execution is needed in the target platform (e.g. I want to get the Elixir version from Elixir to make sure the installation process was Ok) - but most other stuff could be converted to JS... anyway, I've linked this to the newer issue, so your comments will be ported over also. Thanks for the help.
|
Comments done. CI passing. Moving to Ready for review. |
|
@wojtekmach, I'd like to invite you as a reviewer, but the UI won't let me. |
| @@ -2,10 +2,12 @@ | |||
| name: test | |||
There was a problem hiding this comment.
out of the scope of this PR, but out of curiosity what's the difference between ci and test workflows? they have the same "on". should they be different jobs in the same workflow?
There was a problem hiding this comment.
Yeah, they could be different jobs, sure. I never bothered to change this since they were already separated from the beginning. I guess it's for semantics purposes 🤷♂️.
As I see it (but this is me only, I guess) ci will do the "unit tests". test will do something else. (and before these changes one of the two could actually be launched by pressing a button on the UI - it's those _dispatch options).
I can join them. @starbelly, do you feel it's better if these go to the same file now that they're dispatched in the same conditions?
| Pre-release integration tests | ||
| (Windows ${{matrix.combo.os}}, | ||
| Erlang/OTP ${{matrix.combo.otp-version}}, | ||
| rebar3 ${{matrix.combo.rebar3-version}}) |
There was a problem hiding this comment.
here's how it looks like:
(https://github.com/erlef/setup-beam/pull/49/checks?check_run_id=2887537132)
How about:
Test (${{matrix.combo.os}}, Erlang/OTP ${{matrix.combo.otp-version}}, rebar3 ${{matrix.combo.rebar3-version}})
so that at least some part of the matrix shows up in the sidebar?
I'm not sure why rebar3-version didn't show up in the screenshot above.
There was a problem hiding this comment.
This looks better. Let me just get @starbelly's input on the previous question then I'll handle them at the same time.
I'm not sure why rebar3-version didn't show up in the screenshot above.
Me neither: checking... well, in that case we didn't choose a rebar3 on purpose, and I couldn't figure out how to have conditional titles, so that's the best I could do. We could just remove that info and put it in the step name, for example (not tested yet).
| @@ -4698,6 +4726,12 @@ async function maybeInstallElixir(elixirSpec, otpVersion) { | |||
| return true | |||
| } | |||
|
|
|||
| if (shouldMixHex) { | |||
| console.log( | |||
| "hex will not be installed (overriding default) since Elixir wasn't either", | |||
There was a problem hiding this comment.
(overriding default)
what's the default?
I'm a little bit confused about this log message, I'm not sure what it's saying or if it's relevant to people not interested in using Elixir. If the idea was to say that it is possible to install Elixir here, how about something more generic: "no elixir-version specified, skipping Elixir installation"? Remember how we were surprised that ubuntu runtime had it's own rebar3? For that, I think a similar: "no rebar3-version specified, using system provided rebar3" might make sense though I'd leave that for a separate PR.
There was a problem hiding this comment.
ah, I guess this is related to #49 (comment)?
so someone set install-hex: true but left elixir-version out? to me that should be an error: "cannot set install-hex: true without specifying elixir-version" or something.
There was a problem hiding this comment.
Agreed, makes no sense to have one without the other.
There was a problem hiding this comment.
so someone set install-hex: true but left elixir-version out?
The fact is it was a "no-choice". install-hex is true by default (I think it's been like this for a while - didn't check). I can:
- improve the warning/error messages, or
- make it default in the code, not as an input.
Whatever choice I agree this could do with some tweaking for clarity. Let me open a new issue.
There was a problem hiding this comment.
ohhhh, install-hex is true by default, that makes sense. in that case i would honestly silently ignore this and call it a day.
|
@paulo-ferraz-oliveira Did an initial first pass, looks solid! Amazing stuff 🚀 Will review in anger later this evening. |
|
Just dropping by to say how happy I am to see PRs like this. I would not have had the time or probably the expertise personally to get this done (even with outside contributors) while I was maintaining this action under the official Actions org. Thanks again for agreeing to take this on, Erlef folks and thanks for the PR @paulo-ferraz-oliveira! |
|
@paulo-ferraz-oliveira approved, bu CEB7 t I don't understand this question :
|
Co-authored-by: Wojtek Mach <wojtekmach@users.noreply.github.com>
Co-authored-by: Wojtek Mach <wojtekmach@users.noreply.github.com>
We're only going down to 23, but there's installers for previous versions. The thing is those don't follow the same pattern for the "C:\Program Files" path (which, mind you, could also break in the future, since it's not controlled by us). As long as I could be sure that |
|
I'll wait for your comments on the Unresolved conversations, but have started a new issue for a follow-up PR, lest we cram everything here in a single commit 🤣: #51 |
Opening as draft.
As-is, stuff appears to be working (and you're Ok to review, since I don't expect this to change much) but a couple of questions reside. I'll comment on the code later.
Closes #12.
Closes #44.
Edit: one big question for me is "Should we open an exception to the Windows installation script in order to support an even further past"?