-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge setup-erlang in #9
Conversation
Self-review is finished. Bash away. |
I'll make the test fixes in a separate commit so as to not disturb the "base". (I'll keep squashing on top of that one until everything's fine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a first pass on this and left some comments. Great work!
There seems to be some dist/
files and directories that seems duplicated and end with 1
in the name. Can you look into that?
I have looked into that. I could remove them by hand, but then [1] @ericmj, let me know if, locally, you run |
If the files end with Can you try to find the change that is causing the duplicates since we didn't have them before? |
You mean go over |
I can't test locally currently. It looks like the package is deprecated https://www.npmjs.com/package/@zeit/ncc. You can try the package that replaces it. |
We don't have duplicates in |
I thought I'd updated it to (just check, and I did update to |
I agree. Something has changed. But even in |
b58b61c addresses comments:
I'm still missing a follow-up on:
|
I may have traced down the "duplicate |
Why are the test files included? |
I'm not actually sure (since have little knowledge of |
I was able to solve the "duplicate file" thing by completely separating test code from production code (when I started working on this the mix was already there, and I wrongly continued working on top of that). |
Lemme know if the current status is "OK for merge". I'd like to test it in a few projects before moving to |
I will revert
so I fail to see how we can match Edit: I've not yet reverted anything. |
Also, let's assume https://repo.hex.pm/builds/elixir/v1.10.3.zip exists (in which case, Edit: I have local changes prepared to push for an update, but I really want to be sure of what I'm pushing. [1] or rather we might be, but out of coincidence. (to be really sure just change that value to some nonsense like 245 - OTP version). |
@paulo-ferraz-oliveira yeah, I get back 11.1.2 for erlef/website. https://github.com/erlef/website/pull/296/checks?check_run_id=2162938782 . See compile task. Edit: |
I'll want to:
|
I don't know what the context of the regexes are. Are they from the code base or are they the version strings we are supposed to support? |
Sure, my bad. I added a bit of text to the table. Those are the regexps as used by the code to find stuff while parsing the listings, but more generically they attempt at representing the way the different pieces are actually versioned. Edit: e.g. |
What's the reason for filtering certain branches? I don't think we should try to predict what builds will provided so we should support any branch name. |
We're not filtering "branches", but "versions" (that's what I assumed the
Another example: https://github.com/erlef/setup-elixir/blob/main/src/setup-elixir.js#L135 already filters input, but it's probably not required (easier code and easier to reason about which build are available). |
Here we are removing
On the line below we accept any version name. The intention is not to filter here, we are separating the elixir version component and the otp version component from the string. |
Sure, which is why I propose an even further relaxed regexp of "everything until the first space", where the only assumption is the file format will not change, an assumption already in place for the current code.
I know, and understand, but not everything will be a valid version, right? e.g. one that's not in the file (which is why now we crash). Would it not make sense to be more relaxed in the matching here, as well?
Sure. Again, understood, but I think we'd benefit from a more relaxed approach, in any case. As you stated, it's probably the file contents change (including the version format, not controlled by us), but it's less likely the file's format changes, right? I'll propose a new approach to the regex and let you decide, so you have a term of comparison. |
@ericmj, I ended up simplifying the regexp, but not as much as I wanted/thought it would be possible. What I aimed for was complete coverage of the existing build listing (but not making many assumptions). I also added a ton of version combos so we can test it as much as possible. Do let me know if you want a specific version combo (semver, master, whatever, ...) so I can update the tests with that. |
@ericmj, I eventually found the error to be Erlang installed in the container at [1] In the |
Here's a list of the previously failing combos showing that now some pass: https://github.com/paulo-ferraz-oliveira/setup-elixir/runs/2197844972?check_suite_focus=true. (the ones failing, in 18.04 are ALL Also, I've changed the min. Elixir version of the test-project to 1.0? It was preventing functioning CI tests from running. |
That makes sense, since elixir has to support all the way back to 1.0 |
This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has been a big effort and I think the fallout is beautiful. It is extremely well tested (and we actually now see when the tests fail), the js code is so much easier to follow, bugs were found and squashed. All of these improvements will make supporting this 💯 easier. Thus I approve 🚀 ❤️ 💜 💛 💚 🧡 💙
I do think @ericmj needs to take a pass as well before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a second review. Left a few comments, mostly questions, but overall it's a great. Particularly like the extra attention given to testing, we will be more comfortable doing changes in the future with the extensive testing.
__tests__/setup-elixir.test.js
Outdated
} | ||
|
||
async function testOTPVersions() { | ||
const otpBuildsListing = listing('otp_builds.txt') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we require network connection for other tests what about dropping these files and fetching the actual files for simplicity. That way we will also catch any issues with changes to the files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that, sure. I'm just afraid that changing some of these will actually lead to different test results (e.g. I can't use 1
as a version since that'll yield different results as soon as builds.txt
changes), but I can do it, no prob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f79efb2: done. I'm afraid tests (may have) lost some quality, though, as per what I explained previously (if depending on a dynamic environment we can't actually be sure of what'll happen when we do stuff like '^1.10'
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the tests on version ranges that are no longer supported and won't be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what supposedly did, with this change. In any case, to be completely sure, I'd have to know exactly what isn't supported. We can deal with mishaps, if any, later, if new versions come out that change test results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find supported Elixir versions here: https://hexdocs.pm/elixir/compatibility-and-deprecations.html#content. I would guess Erlang has similar documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OT: when you resolve conservations after responding it is very hard to find your replies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OT: when you resolve conservations after responding it is very hard to find your replies.
Sorry about that. It sometimes happens.
Is anything missing regarding this conversation, in particular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Erlang/OTP testing: https://github.com/erlef/setup-beam/pull/9/files#diff-c76af13c0a24fd165fe6700b8b1254b6a48a60b9ddfcb39dcad476a725f8106bR84 (all results belong to no-longer-supported versions)
This is Elixir testing: https://github.com/erlef/setup-beam/pull/9/files#diff-c76af13c0a24fd165fe6700b8b1254b6a48a60b9ddfcb39dcad476a725f8106bR109 (all results belong to no-longer-supported versions)
This is rebar3
testing: https://github.com/erlef/setup-beam/pull/9/files#diff-c76af13c0a24fd165fe6700b8b1254b6a48a60b9ddfcb39dcad476a725f8106bR133 (all results belong to no-longer-supported versions)
I assume we're good (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
src/installer.js
Outdated
if ( | ||
process.platform !== 'linux' && | ||
process.env.CI_TEST_ENV !== 'test' && | ||
process.platform !== 'darwin' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should not be here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should IMO. Locally I should be able to test on a Mac (especially since it works), but if you prefer I can take it out and only assume Linux based testing (will be kinda hindering for updating though - since I have to add it for testing, and remove it before pushing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted this change. It'll simply imply more force-pushes on my behalf to get stuff running properly. No biggie, though. (if somebody runs into issues because of this, in the future, we can reconsider our approach)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work on mac if erlang is compiled against linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup-elixir.test.js
presents two types of tests:
- failing installs (e.g. inexisting file or input not accepted)
- version-based tests (testing against semver)
None of these require the builds to actually run, since we're interested in other conditions, so there's no difference in running them in Linux and Mac, except that without allowprocess.platform
darwin
you can't actually properly run these locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I would recommend unit testing that functionality directly so it doesn't go through this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have e.g. https://github.com/erlef/setup-elixir/pull/9/files#diff-3e35f8d82312f568d749abab56e430c703198c68e3b8468454005ac9bc806797R10. Do you prefer me to test
exec(path.join(__dirname, 'install-otp'), [
osVersion,
otpVersion,
])
directly? (as is, now, it's only affecting local testing, not remote)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move on this front, lest the answer is "yes" and then I lose the opportunity for a further review. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the validation (312b42b) since if we're not doing it for tests there's no need to do it any way (since setup-elixir.js
already does it at start).
Now I should rebase on top of Done: 0b79c31 |
src/installer.js
Outdated
await exec(path.join(__dirname, 'install-elixir'), [version, otpString]) | ||
} | ||
async function installElixir(elixirVersion) { | ||
return await exec(path.join(__dirname, 'install-elixir'), [elixirVersion]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think async
+ return await
is redundant unless you do something like wrap try .. catch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is async
and a single await
also redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... that's a good question. I've always used async
so that consumers await
on my function. If exec
is async
, though, without await
the function should just return "instantaneously".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async function first() {
await exec()
console.log('first')
}
async function exec(Cmd) {
return new Promise((res, rej) => {
setTimeout(function() {
res(console.log('do I show up before "first"?'))
}, 100)
})
}
first()
If your Promise is not synchronous, an await
is required.
This is what MDN states "The body of an async function can be thought of as being split by zero or more await expressions. Top-level code, up to and including the first await expression (if there is one), is run synchronously. In this way, an async function without an await expression will run synchronously. If there is an await expression inside the function body, however, the async function will always complete asynchronously."
It seems like we've got everything ironed out for the most part now. There's probably lots of little things that can be improved on , but I'm not sure this PR is the place for those anymore. Other little things I believe can be handled in subsequent PRs and I think we can and should do some second passes, but the amount of churn on this PR has peaked IMO and is at least for me becoming harder to follow. |
Previously (Remove redundant code) (+27 squashed commits) Squashed commits: [0b79c31] Rename as setup-beam [312b42b] Simplify platform verification [b43ca96] Prevent use of OTP- prefixed Erlang/OTP versions [103a236] Move the promise-based approach to an async-based approach (not testing for failure, so not guaranteeing readable/usable output either) [1e7f631] Remove local test files [d03e0c6] Prevent use of -otp- infixed Elixir versions [697110e] Have versions output in action, not just in tests [7360781] Ease test validations [71e6999] Reinstall tests that were there before [6602f20] Attempt at simplifying regexp matching with build listings [09247af] Provide for an Elixir to "not OTP" fallback mechanism (as explained by the comments) [dfc4083] Attempt at renaming for clarity We also get rid of return values when not reachable We also move the `-otp-` part of the Elixir version upstream so it's easy to reason about (and update the tests accordingly) [0d4eb28] Bump our dep.s (trying to figure out why core.setFailed isn't making the action fail, as it should) [ebf48ef] Have action fail (with an exception) when no expected version is found (we otherwise feel that a simple warning is not enough, as it may end up hiding potential bugs) [8478364] Adapt tests to our actual inputs [16ea63d] Add prettier again, now sync'ed with jslint (pass it on the code, already) [cfc65df] Improve error messages and tests [a017948] Test unavailable Elixir upon mix [7f2a3e9] Test failing installations Also output core.error when expected [08061ca] Attempt at making shell scripts more robust [4ee73cb] Separate test code from production code 1. inputs (for CI) are managed by code, not by .yml env. variables 2. rebar3_builds.txt added for stable repeatability 3. tests tweaked to make it easier to modify and drop dependency on production code 4. `dist` regenerated (duplicate files gone) 5. setup-elixir.js tweaked to this new reality [d17fed4] Remove prettier from the package [7d6f8f5] Actually break CI when we should [b972eaf] Remove unrequired element [0ffa28a] Ease maintenance and readability [4690169] Fix as per broken tests [a1371c6] Merge setup-erlang in
Squashed and force-pushed. Waiting on CI. |
I'll make a self-review after which I welcome yours.
This description may see actions added to it if decisions are yet to be made.
Note: where I previously had
erlang
, I now haveotp
(as per what was present in this action already). Lemme know if I should rename anything.I add:
rebar3
-version -based elements to CIrebar3-version
to the action (everything else remains untouched)rebar3
installation (bash
) script that fetches versions from GitHubconsole
, somecore
)I change:
yarn
-based (thus deleting tons of files from the repo, and adding 3RD PARTY LICENSES)elixir-version
so it becomes optionaltest.yml
ever so slightly in order to make sure backward compatibility is kept while adding new CI stuffaction.yml
ever so slightlyI remove:
licensed
-based license management (and associated assets)All of this means the action should be fully backward compatible.
Also, closes #15.