Rework our version matching algorithm #217

paulo-ferraz-oliveira · 2023-06-23T12:57:02Z

This:

bumps dependencies' versions to their latest, where appropriate
trims GitHub Actions' workflows, where appropriate
trims package.json, where appropriate
tweaks getVersionFromSpec/2 to improve on version matching
adapts test and CI

paulo-ferraz-oliveira · 2023-06-23T12:58:31Z

.github/workflows/action.yml

-      - run: npm install
-      - run: npm run build
-      - run: npm run format
-      - run: npm install -g markdownlint-cli


This was moved to package.json which means this whole npm ... pipeline can simply be as it is proposed here.

paulo-ferraz-oliveira · 2023-06-23T12:59:01Z

.github/workflows/ubuntu.yml

-          - elixir-version: '1.10.3'
-            otp-version: '22.3.4.1'
-            os: 'ubuntu-20.04'


We find this above, already, with version-type: strict.

paulo-ferraz-oliveira · 2023-06-23T12:59:42Z

package.json

-  "husky": {
-    "hooks": {
-      "pre-commit": "npm run format -- --check && npm run build && git add dist/"
-    }
-  },


Not really used, I guess. npm run build-dist does it all.

paulo-ferraz-oliveira · 2023-06-23T13:01:20Z

package.json

-    "eslint-plugin-import": "2.26.0",
-    "eslint-plugin-jsx-a11y": "6.6.1",
-    "eslint-plugin-react": "7.31.10",
-    "husky": "8.0.1",


Not really used, so we can trim down a few deps.

paulo-ferraz-oliveira · 2023-06-23T13:04:01Z

src/setup-beam.js

@@ -184,13 +185,16 @@ async function getElixirVersion(exSpec0, otpVersion0, hexMirrors) {
  const otpVersion = otpVersion0.match(/^([^-]+-)?(.+)$/)[2]
  const otpVersionMajor = otpVersion.match(/^([^.]+).*$/)[1]
  const elixirVersions = await getElixirVersions(hexMirrors)
-  const semverVersions = Array.from(elixirVersions.keys()).sort()


This is now done inside getVersionFromSpec/2, for all consumer functions, as to centralised behaviour and make it more consistent.

paulo-ferraz-oliveira · 2023-06-23T13:04:32Z

src/setup-beam.js

-  return maybePrependWithV(gleamVersion, gleamVersion)
+  return maybePrependWithV(gleamVersion)


The 2nd argument had no value, as it wasn't accepted, in any case.

paulo-ferraz-oliveira · 2023-06-23T13:05:20Z

src/setup-beam.js

@@ -300,7 +309,7 @@ async function getElixirVersions(hexMirrors) {
    .forEach((line) => {
      const elixirMatch =
        line.match(/^v?(.+)-otp-([^ ]+)/) || line.match(/^v?([^ ]+)/)
-      const elixirVersion = maybePrependWithV(elixirMatch[1])


This is no longer required, since internally all versions will be stripped of v and it'll only be added when required for external requests.

paulo-ferraz-oliveira · 2023-06-23T13:07:26Z

src/setup-beam.js

  return rebar3VersionsListing
 }

 function isStrictVersion() {
  return getInput('version-type', false) === 'strict'
 }

-function getVersionFromSpec(spec, versions, maybePrependWithV0) {


Most of the big changes are in this function... and maybeCoerced/1 below.

paulo-ferraz-oliveira · 2023-06-23T13:07:51Z

src/setup-beam.js

  }

-  return v
+  return version || null
 }

 function maybeCoerced(v) {


Most of the big changes are in this function... and getVersionFromSpec/2 above.

starbelly · 2023-06-24T16:19:58Z

It is my experience that it's rare the development community at large targets versions like 25.3.2.1, preferring to stay at the 25.3.x level.

Yeah, I agree with that, but the fact remains still that even 25.3.1 is not semver in the case of OTP. However, I think it's fair to say it can be treated like that. I would agree this is a breaking change. So, we're thinking this would result in a new major version then, is that correct?

paulo-ferraz-oliveira · 2023-06-24T21:20:06Z

It'd probably result in a breaking change, yes, depending on:

whether we consider it to be a bug (which it is since we've not been handling it properly), or
we consider it to be a new feature we want the action to present.

I "think" I can try to fit the x.y.z.a versions in the mix (maybe?), but I tried the other day and the solution got so convoluted I rolled back some decisions: I also don't like that I'm forced to write 25.3.2.2, strict, because my consumer's expectation is 25.3.2 is whatever is before 25.4.0, but... Now I got (merge) conflicts, I got solve those and we can continue discussing this to see what our common expectation is.

Edit: Ok, so I'm trying a new approach where I compare version suffixes to a fixed version.

paulo-ferraz-oliveira · 2023-06-25T00:01:44Z

I rebased to resolve merge conflict and increased complexity for getVersionFromSpec/2 (hopefully not by much) so we can keep previous behaviour but make the code easier to maintain, also...

Edit: also, since the last commit, I moved back most changes in the .test.js module, so we make sure whatever was there works as-was, but we have new behaviour working properly too.

paulo-ferraz-oliveira · 2023-06-25T00:02:46Z

src/setup-beam.js

-  if (isVersion(otpSpec0)) {
-    otpSpec = `OTP-${otpSpec0}` // ... it's a version!
-  }


The result of this block was never used. I don't know why static analysis wouldn't find this...

Needs hank.

paulo-ferraz-oliveira · 2023-06-25T00:04:17Z

src/setup-beam.js

+function xyzabVer(pref, suf) {
+  // This accounts for stuff like 6.0.2.0.1.0, as proposed by Erlang's
+  // https://www.erlang.org/doc/system_principles/versions.html
+  const dd = '\\.?(\\d+)?'
+  return new RegExp(
+    `${pref}v?(\\d+)${dd}${dd}${dd}${dd}${dd}(?:-rc\\.?\\d+)?(?:-otp-\\d+)?${suf}`,
+  )
+}
+


This function, and its callers, will probably be the "most difficult bits" to maintain, but the regexps here are pretty relaxed, too...

paulo-ferraz-oliveira · 2023-07-19T23:11:58Z

@starbelly?

starbelly · 2023-07-19T23:29:37Z

Oof! Conflicts @paulo-ferraz-oliveira

starbelly

@paulo-ferraz-oliveira This LGTM. I'd be in favor of merging and a delay on the release, sound good?

paulo-ferraz-oliveira · 2023-07-21T19:48:14Z

Sure. We can merge and delay release. That'll give me a chance to update the other pull request I have opened.

We want the most important things to fail first and prevent the other ones from executing e.g. if your dist/ is not updated, there's no point seeing the tests fail

We'll deal with all this in a centralized place to make it easier to reason on the code

1. stop accepting maybePrependWithV0, since this is only used in specialized cases 2. sort versions internally (don't expect it to come sorted) 3. compare rc/strict and .includes in a single go (don't separate these) 4. don't expect already coerced version arrays, coerce them when required (the maybeCoerced version had changed already, which means we're smarter at putting stuff inside the version array) 5. raise an exception if a semver-invalid version is not flagged as strict

This reverts commit 500b0fe.

This reverts commit fbe1317.

We cheat on the semver matching: 1. we keep an ordered list of versions that respect e.g. 22.3.4 2. if we match 22.3.4 when fetching a version, we get the last value from the list of versions that respect it, e.g. 22.3.4.2.1.0 (since this is ordered, it should work as previously expected) This increases code complexity for getVersionFromSpec, but hopefully not by much... We also remove throw-based test cases (non-versions will fail later, with null), and add some tests to prove our need code.

But still keep them useful

paulo-ferraz-oliveira · 2023-07-21T19:52:04Z

Force-pushing to resolve merge conflicts. If no actions show issues I'm squash-merging.

josevalim · 2024-01-17T14:18:19Z

Thank you @paulo-ferraz-oliveira! It seems this PR removed support for maint/master from OTP git, which Elixir relies on. Was it intentional?

josevalim · 2024-01-17T14:21:50Z

Nevermind, PR sent here: #241 :)

paulo-ferraz-oliveira · 2024-01-17T20:05:04Z

It seems this PR removed support for maint/master from OTP git, which Elixir relies on. Was it intentional?

👋 It wasn't. I'll look at the pull request.

paulo-ferraz-oliveira requested review from ericmj and starbelly June 23, 2023 12:57

paulo-ferraz-oliveira commented Jun 23, 2023

View reviewed changes

paulo-ferraz-oliveira force-pushed the feature/minor-tweaks branch 2 times, most recently from 9d941d8 to 184788e Compare June 24, 2023 21:40

paulo-ferraz-oliveira changed the title ~~Restrict input to SemVer, unless version-type: strict~~ Rework our version matching algorithm Jun 24, 2023

paulo-ferraz-oliveira commented Jun 25, 2023

View reviewed changes

paulo-ferraz-oliveira force-pushed the feature/minor-tweaks branch 5 times, most recently from 7f23a6f to 9f89e26 Compare June 25, 2023 04:43

paulo-ferraz-oliveira mentioned this pull request Jun 30, 2023

What is the OTP26 binary? #220

Closed

paulo-ferraz-oliveira force-pushed the feature/minor-tweaks branch from 283c56d to ac530ac Compare July 1, 2023 16:57

paulo-ferraz-oliveira mentioned this pull request Jul 3, 2023

Feature tool-cache for Gleam and rebar3 #223

Merged

2 tasks

paulo-ferraz-oliveira requested review from starbelly and removed request for starbelly July 10, 2023 21:26

starbelly approved these changes Jul 19, 2023

View reviewed changes

paulo-ferraz-oliveira added 20 commits July 21, 2023 20:49

Update some dependencies' versions (also, get rid of husky)

7e372b1

Introduce branch dependency execution

0724187

We want the most important things to fail first and prevent the other ones from executing e.g. if your dist/ is not updated, there's no point seeing the tests fail

Rid ourselves of bloat

b49c93c

Introduce expectations for upcoming version

b324365

Tweak whitespace

40fd5f4

Tweak our calls to getVersionFromSpec (increase code consistency)

5cd8299

Remove non-used variable

ab34b3b

Start coercing on input, stop sorting/maybe prepending 'v' on input

d562423

We'll deal with all this in a centralized place to make it easier to reason on the code

Reduce our expectations, but still be appropriate to our mission

fa70a82

Update our tests for our new expectations (start the TDD bit)

e06852f

Execute npm run build-dist and keep results

0a96fb1

Increase inter-job dependency

4960d20

Don't expect markdownlint to be installed remotely

79a27d8

Revert "Increase inter-job dependency"

0f04e6e

This reverts commit 500b0fe.

Revert "Introduce branch dependency execution"

3e94d49

This reverts commit fbe1317.

Have all the CI workflows run under the same conditions

b210439

Fix as per CI results

bd45a7f

Trim out linting practices

6ba70af

But still keep them useful

paulo-ferraz-oliveira force-pushed the feature/minor-tweaks branch from ac530ac to 6ba70af Compare July 21, 2023 19:51

paulo-ferraz-oliveira merged commit f6a73a7 into erlef:main Jul 21, 2023
63 checks passed

paulo-ferraz-oliveira deleted the feature/minor-tweaks branch July 21, 2023 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework our version matching algorithm #217

Rework our version matching algorithm #217

paulo-ferraz-oliveira commented Jun 23, 2023 •

edited

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

paulo-ferraz-oliveira Jun 23, 2023

starbelly commented Jun 24, 2023

paulo-ferraz-oliveira commented Jun 24, 2023 •

edited

paulo-ferraz-oliveira commented Jun 25, 2023 •

edited

paulo-ferraz-oliveira Jun 25, 2023

starbelly Jul 19, 2023

paulo-ferraz-oliveira Jun 25, 2023 •

edited

paulo-ferraz-oliveira commented Jul 19, 2023

starbelly commented Jul 19, 2023

starbelly left a comment

paulo-ferraz-oliveira commented Jul 21, 2023

paulo-ferraz-oliveira commented Jul 21, 2023

josevalim commented Jan 17, 2024

josevalim commented Jan 17, 2024

paulo-ferraz-oliveira commented Jan 17, 2024

		return maybePrependWithV(gleamVersion, gleamVersion)
		return maybePrependWithV(gleamVersion)

Rework our version matching algorithm #217

Rework our version matching algorithm #217

Conversation

paulo-ferraz-oliveira commented Jun 23, 2023 • edited

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

starbelly commented Jun 24, 2023

paulo-ferraz-oliveira commented Jun 24, 2023 • edited

paulo-ferraz-oliveira commented Jun 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulo-ferraz-oliveira Jun 25, 2023 • edited

Choose a reason for hiding this comment

paulo-ferraz-oliveira commented Jul 19, 2023

starbelly commented Jul 19, 2023

starbelly left a comment

Choose a reason for hiding this comment

paulo-ferraz-oliveira commented Jul 21, 2023

paulo-ferraz-oliveira commented Jul 21, 2023

josevalim commented Jan 17, 2024

josevalim commented Jan 17, 2024

paulo-ferraz-oliveira commented Jan 17, 2024

paulo-ferraz-oliveira commented Jun 23, 2023 •

edited

paulo-ferraz-oliveira commented Jun 24, 2023 •

edited

paulo-ferraz-oliveira commented Jun 25, 2023 •

edited

paulo-ferraz-oliveira Jun 25, 2023 •

edited