Allow version-type: strict + otp-version: master for Elixir -based builds #144

paulo-ferraz-oliveira · 2022-10-02T19:55:55Z

(edit: though the title is somewhat restrictive I believe this fix should work for Gleam -based builds in the same way)

@ericmj found that we can't use combo

  otp-version: master
  elixir-version: 1.14.0-otp-master

but this is unintentional, as we should, with version-type: strict. This pull request aims to fix that.

GitHub actions is refusing to run a given workflow :shrug

Which is exactly what we intended to originate by starting a new pull request.

paulo-ferraz-oliveira · 2022-10-02T20:34:00Z

We add a workflow -based test and a unit test for this change. Since I'm doing only additions to the code, I'm pretty confident there's no regressions (believing that all previous tests were already very comprehensive).

Actions' window shows everything as Ok but then internals not Error when evaluating 'runs-on' for job 'integration_test'. .github/workflows/ubuntu.yml (Line: 19, Col: 14): Unexpected type of value '', expected type: OneOf. Strange one, this one!

paulo-ferraz-oliveira · 2022-10-02T20:43:03Z

Ah, wait, spoke too soon. This appears to have some kind of influence of previously working tests. Now I just need to make sure the tests were meaningful and correct as per this change.

paulo-ferraz-oliveira · 2022-10-02T20:58:39Z

I guess strict still tries to do some guess work in unexpected cases. I'll fix that.

We do this for OTP-, but not for maint- as the former is hopefully more common than the latter

paulo-ferraz-oliveira · 2022-10-02T21:26:57Z

@ericmj, what should happen if a developer chooses Elixir master but OTP 25? As opposed to Elixir master and OTP 24? In the latter we know how to choose master-otp-24, but how should we know to only choose master if OTP is 25 (i.e. without reading the whole builds.txt and deducing that)?

(edit: from #134 (comment) I'd already asked "Should we open an exception, in the code, for master? I see that -otp-25 only shows up some times, so not sure what the rule is, here." for a lack of rule, but I'm still wondering how we could improve this).

(edit 2: also, I'm not sure if Elixir master is OTP 25 or OTP master, in which case master/master, in the input, could be reduced to master alone).

(edit 3: we could also implement OTP version as latest which would imply master but no suffix to the Elixir version)

(edit 4: error is specifically here https://github.com/erlef/setup-beam/actions/runs/3170320256/jobs/5162830251#step:3:16; we try to fetch 25 and end up with maint-25, but even if we were to end up with OTP-25 there'd be no easy-to-find corresponding Elixir version - i.e. how do we specify 1. Elixir master with OTP master and 2. Elixir master and OTP 25?)

Semantically they're similar, but with `latest` we know: 1. to fetch Elixir's no-otp-... version 2. to fetch Erlang's master whereas with 25 (latest at this moment) we'd try to fetch elixirvsn-otp-25 which could fail with Elixir master and version-type strict

Still not 100% convinced that `latest` should be separate from `master` but I want to know how tests run, in this case in particular

@ericmj

We introduce the concept of version v. branch (as per @ericmj) and we use that to find the most appropriate Erlang/Elixir combo.

paulo-ferraz-oliveira · 2022-10-04T00:08:19Z

.github/workflows/ubuntu.yml

@@ -21,6 +21,13 @@ jobs:
      fail-fast: false
      matrix:
        combo:
+          - otp-version: 'master'


Added as part of the "was failing, but shouldn't!" testbed.

paulo-ferraz-oliveira · 2022-10-04T00:08:58Z

.github/workflows/ubuntu.yml

@@ -113,12 +120,13 @@ jobs:
          - elixir-version: 'master'
            otp-version: '23.1'
            os:  'ubuntu-20.04'
-          - elixir-version: 'master'
+          - elixir-version: 'main'


This was actually a fix. Since OTP 25, Elixir no longer has master, but main.

paulo-ferraz-oliveira · 2022-10-04T00:11:32Z

.github/workflows/ubuntu.yml

            otp-version: '25'
            os:  'ubuntu-20.04'
            version-type: 'strict'
          - gleam-version: 'nightly'
            otp-version: '24'
+            os: 'ubuntu-latest'


I believe this should've been broken, but can't understand how we didn't see it before. In any case, on the README, the Ubuntu build is broken and there were recently issues with running actions on GitHub actions like so, which was hiding bugs.

paulo-ferraz-oliveira · 2022-10-04T00:12:01Z

__tests__/setup-beam.test.js

@@ -205,9 +213,9 @@ async function testElixirVersions() {
  simulateInput('version-type', 'loose')

  simulateInput('version-type', 'strict')
-  spec = 'master'
+  spec = 'main'


Explained above, though probably only meaningful for OTP 25+

paulo-ferraz-oliveira · 2022-10-04T00:12:28Z

dist/index.js

@@ -7189,8 +7189,8 @@ async function main() {
  const rebar3Spec = core.getInput('rebar3-version', { required: false })

  if (otpSpec !== 'false') {
-    const otpVersion = await installOTP(otpSpec, osVersion)


We install Elixir based on the direct OTP input, and not the output of this function (easier to reason about)...

paulo-ferraz-oliveira · 2022-10-04T00:13:04Z

dist/index.js

-      otpSpec[2],
-      Array.from(otpVersions.keys()).sort(),
-    )
+  let otpSpec = otpSpec0 // might be a branch (?)


Greatly simplified. We drop OTP-, maint- and whatnot to favour "branch over version," as suggested by @ericmj.

paulo-ferraz-oliveira · 2022-10-04T00:13:42Z

dist/index.js

@@ -7324,60 +7318,46 @@ async function getOTPVersion(otpSpec0, osVersion) {
  return otpVersions.get(otpVersion) // from the reference, for download
 }

-async function getElixirVersion(exSpec0, otpVersion) {
+async function getElixirVersion(exSpec0, otpVersion0) {


Also greatly simplified as per previous explanation.

paulo-ferraz-oliveira · 2022-10-04T00:14:31Z

dist/index.js

@@ -7438,7 +7420,6 @@ async function getOTPVersions(osVersion) {
        })
    })
  }
-


Unintended...

paulo-ferraz-oliveira · 2022-10-11T12:20:30Z

@ericmj, did you get around to testing this?

paulo-ferraz-oliveira · 2022-10-16T22:10:37Z

@starbelly, I'm recruiting your help to validate these changes, as @ericmj might not be available at this moment. They are not only pertinent to the initial problem issued by @ericmj, but also generally to improve the code base. I can walk you through the changes if there's something you don't understand or need further detail on.

starbelly · 2022-10-17T23:14:58Z

The changes look reasonable, let me give this a whirl in a test project tomorrow.

starbelly · 2022-10-19T00:00:58Z

src/setup-beam.js

-        "should not contain '-otp-...'",
-    )
+  const exSpec = exSpec0.replace(/-otp-.*$/, '')
+  const elixirVersionFromSpec = getVersionFromSpec(exSpec, semverVersions, true)


This is the only thing I don't like, the boolean blindness, but it's javascript so 🤷 😁 The only thing I could thing to do improve this is to pass around an object, I would not block on it though.

starbelly

@paulo-ferraz-oliveira This looks solid to me. Tested and all. Since it's not a small change, we may want to hold off on force pushing it the v1 tag and maybe ask some other to try it for a bit. I'll certainly give it a solid whirl at work for a while. What do you think?

Regardless, approved 🎯

ericmj

Thank you @paulo-ferraz-oliveira. This works perfectly https://github.com/hexpm/hex/actions/runs/3284596299.

paulo-ferraz-oliveira · 2022-10-19T20:08:36Z

@starbelly, I can push the minor and patch only, but will need a comment from you, or anybody else, in a few days, to force-push v1.

paulo-ferraz-oliveira · 2022-10-19T20:52:47Z

v1.14 and v1.14.0 pushed. I'll create the release now. @starbelly, do let us know soonish, so we can force push v1. Thanks.

starbelly · 2022-10-20T16:24:53Z

@paulo-ferraz-oliveira Ship it!! 🥳

paulo-ferraz-oliveira added 6 commits October 2, 2022 20:54

Check tests failing for something that should pass

732db5f

Test with version-type: strict

eb570c2

Add more tests

495fd29

GitHub actions is refusing to run a given workflow :shrug

Try to fix new test that shouldn't be failing

9376ade

Which is exactly what we intended to originate by starting a new pull request.

Fix tests as per CI

a99cccf

Fix tests as per CI

b45468e

paulo-ferraz-oliveira changed the title ~~Check tests failing for something that should pass~~ Allow version-type: strict + otp-version: master for Elixir -based builds Oct 2, 2022

paulo-ferraz-oliveira requested a review from ericmj October 2, 2022 20:32

paulo-ferraz-oliveira added 2 commits October 2, 2022 21:36

Adapt to "latest"

452a06b

Fix as per CI results

13ca1c5

Actions' window shows everything as Ok but then internals not Error when evaluating 'runs-on' for job 'integration_test'. .github/workflows/ubuntu.yml (Line: 19, Col: 14): Unexpected type of value '', expected type: OneOf. Strange one, this one!

Try to "help" for non-OTP declared input

84950cf

We do this for OTP-, but not for maint- as the former is hopefully more common than the latter

paulo-ferraz-oliveira added 7 commits October 2, 2022 22:41

Fix as per CI

cea2912

Still not 100% convinced that `latest` should be separate from `master` but I want to know how tests run, in this case in particular

Fix as per CI

2b584d6

Fix as per CI

e7fee2d

Revert potential wrong decision

e9c3877

Fix as per understand recent Elixir changes (namely master v. main)

bb91f6d

Try to simplify it

206ca1a

We introduce the concept of version v. branch (as per @ericmj) and we use that to find the most appropriate Erlang/Elixir combo.

paulo-ferraz-oliveira commented Oct 4, 2022

View reviewed changes

paulo-ferraz-oliveira requested a review from starbelly October 16, 2022 22:09

starbelly reviewed Oct 19, 2022

View reviewed changes

starbelly approved these changes Oct 19, 2022

View reviewed changes

ericmj approved these changes Oct 19, 2022

View reviewed changes

paulo-ferraz-oliveira mentioned this pull request Oct 19, 2022

Force push v1 to v1.14.0 #149

Closed

paulo-ferraz-oliveira merged commit 8d31e4d into erlef:main Oct 19, 2022

paulo-ferraz-oliveira deleted the feature/otp-as-master branch October 19, 2022 20:12

paulo-ferraz-oliveira mentioned this pull request Oct 19, 2022

Move away from set-output (while updating all deps) #148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow version-type: strict + otp-version: master for Elixir -based builds #144

Allow version-type: strict + otp-version: master for Elixir -based builds #144

paulo-ferraz-oliveira commented Oct 2, 2022 •

edited

Loading

paulo-ferraz-oliveira commented Oct 2, 2022

paulo-ferraz-oliveira commented Oct 2, 2022

paulo-ferraz-oliveira commented Oct 2, 2022

paulo-ferraz-oliveira commented Oct 2, 2022 •

edited

Loading

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira Oct 4, 2022

starbelly Oct 18, 2022

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira Oct 4, 2022

paulo-ferraz-oliveira commented Oct 11, 2022

paulo-ferraz-oliveira commented Oct 16, 2022

starbelly commented Oct 17, 2022

starbelly Oct 19, 2022

starbelly left a comment

ericmj left a comment

paulo-ferraz-oliveira commented Oct 19, 2022

paulo-ferraz-oliveira commented Oct 19, 2022

starbelly commented Oct 20, 2022

@@ @@ -7438,7 +7420,6 @@ async function getOTPVersions(osVersion) { @@
                       })
                   })
                 }

Allow version-type: strict + otp-version: master for Elixir -based builds #144

Allow version-type: strict + otp-version: master for Elixir -based builds #144

Conversation

paulo-ferraz-oliveira commented Oct 2, 2022 • edited Loading

paulo-ferraz-oliveira commented Oct 2, 2022

paulo-ferraz-oliveira commented Oct 2, 2022

paulo-ferraz-oliveira commented Oct 2, 2022

paulo-ferraz-oliveira commented Oct 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulo-ferraz-oliveira commented Oct 11, 2022

paulo-ferraz-oliveira commented Oct 16, 2022

starbelly commented Oct 17, 2022

Choose a reason for hiding this comment

starbelly left a comment

Choose a reason for hiding this comment

ericmj left a comment

Choose a reason for hiding this comment

paulo-ferraz-oliveira commented Oct 19, 2022

paulo-ferraz-oliveira commented Oct 19, 2022

starbelly commented Oct 20, 2022

paulo-ferraz-oliveira commented Oct 2, 2022 •

edited

Loading

paulo-ferraz-oliveira commented Oct 2, 2022 •

edited

Loading