Test for updated doc. on latest / ranges / -rc
#349
Conversation
d7d8283 to
6be4922
Compare
|
@Schultzer, I'm looking at #269 to find a reference for the change proposed in the PR. Can you remember what this was for: https://github.com/erlef/setup-beam/pull/269/files#diff-6406ae11684199b17d2cdca1efe2cfa29fb4587c3b142f1d33ad5f81a5acf7adR388-R389? Edit: I also think there's something strange with https://github.com/erlef/setup-beam/pull/269/files#diff-86f99337fc28b990485e054ebd09d9d3c29ec10775791de7abd2f1a85e17e241R611-R619. I think you're wrongly getting |
|
Also, fwiw, I'm getting a match for Edit: I think we simply didn't count on |
|
This just seems wrong > semver.coerce('maint-27')
SemVer {
options: {},
loose: false,
includePrerelease: false,
raw: '27.0.0',
major: 27,
minor: 0,
patch: 0,
prerelease: [],
build: [],
version: '27.0.0'
} |
Yes, Rebar has both
The ubuntu version are different, one has the rc version the other not. You can take a look in the test data to verify. Also I don’t believe we implemented logic for different main/maint versions for latest as it does not make sense in a release context. |
What is wrong with this, it’s parsed according to spec. You can try the loose option, it might preserve the maint part.
There might be a bug in the version range implementation, I think the simplest solution would be to filter the OTP versions of main out if main is not specifically specified and if main is specified coerce and sort. It’s possible to use semver sorting functions to sort the array. |
Cool. I'm gonna add a comment for this. |
When there's |
8000
src/setup-beam.js
Outdated
| @@ -474,8 +483,8 @@ function getVersionFromSpec(spec0, versions0) { | |||
| const altVersions = {} | |||
| Object.entries(versions0).forEach(([version, altVersion]) => { | |||
| let coerced | |||
| if (isStrictVersion() || isRC(version)) { | |||
| // If `version-type: strict` or version is RC, we just try to remove a potential initial v | |||
| if (isStrictRCOrKnown(spec)) { | |||
There was a problem hiding this comment.
I’m on mobile, but it looks to me that this is the change that is causing the maint issue, as the logic has changed from isStrict or isRC. And I’m not sure why we would allow known branches here.
But if we are, then we need to refactor our implementation to sort and pick the right version, obviously the version range would need to be something like > maint.
But I’m not sure supporting branches in version ranges even makes sense, in my mind if you choose to test on maint you properly want to be strict with your versions.
There was a problem hiding this comment.
I’m in mobile, but it looks to me that this is the change that is causing the maint issue
There's still no issue; I'm testing as I go.
There was a problem hiding this comment.
And I’m not sure why we would allow known branches here.
Well, the else is "maybe coerce it", and since known branches can't be coerced, the result is the same as changing that bit.
|
Spec states Doesn't this mean that it should make |
Yes you are correct, one of the matrix has 27 with is the most of the listed versions. |
When we coerce then we take something that might not be valid semver and make it into valid semver. If you wanted to check if the the version is valid semver you should use valid function. |
b676b81 to
623cda0
Compare
623cda0 to
abc3213
Compare
You are correct. It's per spec for the |
|
Tests are passing. I'm gonna make this Ready for review. I'll then point out, in the diff. elements, via comments, stuff we can discuss in threads, if any. |
f6334dc to
200b93b
Compare
There was a problem hiding this comment.
I thoroughly revisited this as a way to make it as clear as possible... please give it a read and lemme know otherwise.
| if ( | ||
| isStrictVersion() || | ||
| isRC(version) || | ||
| isKnownBranch(version) || | ||
| isKnownVerBranch(version) | ||
| ) { |
There was a problem hiding this comment.
These are the exception we use when building the comparison list. We don't want maint-27 in the same bucket as 27.0, for example. Or even 27.0-rc3 since we state these are opt-in and latest already takes care of that.
| if ( | ||
| isStrictVersion() || | ||
| isRC(spec0) || | ||
| isKnownBranch(spec0) || | ||
| isKnownVerBranch(spec0) | ||
| ) { |
There was a problem hiding this comment.
Similarly, when the input is strict, and rc, ... we don't need to compare it in a range (done below), so we skip that entirely.
| assert.deepStrictEqual(got, expected) | ||
|
|
||
| spec = '> 0' | ||
| expected = 'OTP-26.2.5' |
There was a problem hiding this comment.
Below we can see that latest is with RC, but considering a range is without it.
|
@starbelly, I'll also leave it open for you to say something, but will eventually merge. |
|
I tried latest main with name: Main
on:
push:
jobs:
elixir-latest:
runs-on: ubuntu-24.04
steps:
- uses: erlef/setup-beam@6a38171948a866c69ed72a5c19102478ec2ab81f
with:
elixir-version: "> 0"
otp-version: "28.0.1"
- run: |
elixir --version
otp-latest:
runs-on: ubuntu-24.04
steps:
- uses: erlef/setup-beam@6a38171948a866c69ed72a5c19102478ec2ab81f
with:
elixir-version: "1.18.4"
otp-version: "> 0"
- run: |
elixir --versionThe first job succeeded however it picked latest unstable release: The second job failed: |
Why is it installing Elixir rc? Would be great to see which versions of elixir and otp the action is picking. Also, can you link to the actions? |
|
Hm... that's odd. The tests show differently. Do you see the issue with the latest released version? Fwiw, here's a test: https://github.com/erlef/setup-beam/pull/349/files#diff-86f99337fc28b990485e054ebd09d9d3c29ec10775791de7abd2f1a85e17e241R883 and considered versions are: |
|
If you meant erlef/setup-beam@v1, same result: https://github.com/wojtekmach/setup_beam_bug/actions/runs/15912266022 |
|
Lemme try to load the same |
|
Oh, yeah, that's most likely because the tests don't go to the same interface. 😢 They use a lower level version, whereas your use case is crashing "above" it. |
|
Actually, if you look closely, Erlang installed. It's just that we're using the same input for Elixir and it crashes there... lemme try something. |
|
@wojtekmach, if you use a range for Elixir but a "regular" version for Erlang, does it work? |
Interesting, I wonder what it would pick if both elixir and otp used the version range. I’m not on my computer, but I would imagine that there is some kind of issue in our implementation for version range or maybe the sorted versions. Since per semver spec you should never get a rc. I’m not sure but this build name might be misleading v1.19-otp-28 since it does not contain rc. |
That's: right? Or you had different values in mind? |
|
That should do. |
Since Erlang installs first, if we use the result of that for Elixir version we should be Ok. But we're using direct input from the action. We probably shouldn't. |
|
That was the first example! |
I guess... |
Sorry, I missed there were two, since I was already looking at the error. I think we can get around it by not trying to recalculate OTP major from consumer input, but just using what's there from the |
|
I got good results in my latest implementation. Now I just gotta figure out how to mock the inputs, because the functions that actually get the versions rely on remote. |
|
This was released in v1.20.2. |
Description
I'm trying to bring some clarity to #321, with the following in mind:
README.mdCloses #321.