Fix version ranges for non-semver

[paulo-ferraz-oliveira]

This is an attempt at trying to understand why OTP 24 is not available yet. Opening as a draft as most likely this investigation will result is a closed-unmerged branch.

[paulo-ferraz-oliveira]

Hm...

• semver.validRange('24') returns '>=24.0.0 <25.0.0-0', which is seemingly Okay (24 is the input spec),
• semver.maxSatisfying(['24.0'], '>=24.0.0 <25.0.0-0') returns null, which is not usable (24.0 is the value found in the Hex.pm listing).

Some might say semver is wrong, here.

Thoughts on how to solve this?

Edit: I tried update semver (npm dep. but this is still no good).

[paulo-ferraz-oliveira]

Lets try with 24.0, instead of 24: 51e8d2f.

[paulo-ferraz-oliveira]

This appears to not be the best option. Waiting for input.

[paulo-ferraz-oliveira]

Moving from Draft, for visibility. Not merge-ready yet.

[ericmj]

OTP 24 is not available because Bob has not built it yet. There seems to be a regression in OTP 24 that causes the compilation to fail.

[ericmj]

OTP 24 is not available because Bob has not built it yet. There seems to be a regression in OTP 24 that causes the compilation to fail.

It seems it only fails for Docker images, so the issue is unrelated.

[paulo-ferraz-oliveira]

@ericmj, I've tracked it down to this being a semver (npm package) issue. We either:

• force 24 to be 24.0 (e.g. in a hacks module), or
• we create a more generic rule (e.g. x becomes x.0), or
• we fix semver, or
• we find an alternative to semver.

I'd prefer to not fiddle with our code, since its assumptions are correct, and either find an alternative to semver or open an issue to fix it. I'll start be opening the issue, but accept thoughts 😄

[paulo-ferraz-oliveira]

Bug opened: npm/node-semver#382.

[ericmj]

I don't believe it's a bug in the semver package but rather that OTP versions are not semver. 24.0 is not valid semver so it's understandable that semver.maxSatisfying does not accept it.

We could try to convert 24.0 => 24.0.0 so that the value is accepted.

Personally I don't think we should support version ranges because CI should be reproducible and automatically updating versions are not reproducible but I guess we need to maintain the backwards compatibility. Did this work before the setup-beam rewrite?

[paulo-ferraz-oliveira]

I don't believe it's a bug in the semver package but rather that OTP versions are not semver. 24.0 is not valid semver so it's understandable that semver.maxSatisfying does not accept it.

Understandable, yes; that it won't/shouldn't accept it... I'm still waiting for a reply to the issue.

Personally I don't think we should support version ranges because CI should be reproducible

Agree almost completely; this is from the "setup-elixir" times. We offer this flexibility for people who don't care about repeatability (I guess!), also, and because otherwise this action could just be a couple of bash scripts.

and automatically updating versions are not reproducible

That's the main issue I have with the ranges (non-reproducibility). At the same time, some will use branches in their dependencies, which also break this promise, but on our end, not supporting ranges could be an extra safety net.

but I guess we need to maintain the backwards compatibility

We don't need to. As per semver we just go 2.x (and add some kind of release note) and that's it 😄

Did this work before the setup-beam rewrite?

Most surely not, as per https://github.com/actions/setup-elixir/blob/main/src/setup-elixir.js#L104, which was left untouched, algo-wise.

[paulo-ferraz-oliveira]

I find that semver.minVersion(spec).version for spec=24 returns 24.0.0, as well as for spec=24.0, so I'm going with that, for now.

Mind you I changed the 24.0 in the tests back to 24 to test this particular change.

Edit: breaks for e.g. 19.3.x
Edit: I'm going to try with this loose option. Mind you the README already makes the following explicit: "For best results, we recommend specifying exact Erlang/OTP, Elixir versions, and rebar3 versions"

[paulo-ferraz-oliveira]

I've squash-rebased all of this. Waiting for approval and merging...

[paulo-ferraz-oliveira]

@starbelly, @ericmj, @wojtekmach, could I ask your 👀 on this so we can merge it and ship 1.7.1? I'd want to pull this to a couple of repo.s, already. Thanks much.

If you have a better, even less verbose for example, "fix", do tell. I'd like to potentially consider dropping the ver-ranges, but not in this pull request.

[paulo-ferraz-oliveira]

I pushed some more tests for the newly introduced code based on no-longer maintained versions (so it won't easily break in a near future).