-
-
Notifications
You must be signed in to change notification settings - Fork 545
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
GitHub actions asset build #1078
base: master
Are you sure you want to change the base?
Conversation
b5bfeaa
to
5e88966
Compare
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 review more of this later, but this is awesome! I was thinking about doing some of this myself, so I'm glad to see it being started!
9ce8850
to
361a8c5
Compare
.github/workflows/build-asset.yml
Outdated
name: integration-tests-musl-${{ matrix.os }}-${{ env.NODEJS_VERSION }}${{ env.EXECUTABLE_SUFFIX }} | ||
path: integration-tests | ||
if-no-files-found: error | ||
- if: startsWith(github.ref, 'refs/tags/') |
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 was just a guess at a plan for how to handle this :-) Similar for the defaulting to prerelease below too :-)
86fd91f
to
3f311a9
Compare
with: | ||
fail_on_unmatched_files: true | ||
prerelease: true | ||
files: ${{ env.NEXE_ASSET }} |
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.
TODO: The name of this file probably doesn't match the existing pattern
As a thought, would be keen to get this landed (if it looks okay, etc :-) ) before then following up with a PR to use the assets, rather than trying to get it all together in one PR :-) Partly to help keep the PRs (and effort) smaller and more manageable, but also to get the test coverage in asap :-) Knowing about Node.js versions failing is really nice :-) |
3f311a9
to
ab96768
Compare
@@ -1,73 +0,0 @@ | |||
schedules: |
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.
Thanks for starting this! Originally actions didn't work because of the timeout so I had used azure pipelines. This will be so much better.
18ceea4
to
a570eff
Compare
a570eff
to
daff50d
Compare
daff50d
to
8a2c977
Compare
Hi @bruce-one! Fantastic job! Thx! |
8a2c977
to
cae0b70
Compare
On macOS the temp path is a symlink and this was making the `/snapshot` resolution (`this.root` vs `/snapshot` in the zipFs) not work properly. This feels a bit like a workaround, but the path being different to the executable was the culprit in the tests not working. As that's manual code in the integration test script, and due to the custom entrypoint for Mocha, that makes this not a workaround :-)
cae0b70
to
3c2df0d
Compare
This shouldn't make the test execution fail. It seems to popup occasionally for some Windows removal racing, or similar.
3c2df0d
to
a7edf80
Compare
What this PR does / why we need it:
Adding GitHub actions build for creating the nexe asset, and also using it with the integration tests to ensure it works.
Such that people don't need to do
--build
all the time :-)Work in progress :-)
Also, this showed that Node 18.19.0 seems to be broken, and have a fix for that (in theory), so will split that off at some point :-)