| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Generally LGTM
My understanding is that changes to actions don't unless the PR is coming from a local branch, how should we go about properly testing this?
Sorry, something went wrong.
There was a problem hiding this comment.
do we want to run this on windows / linux too?
Sorry, something went wrong.
There was a problem hiding this comment.
@MylesBorins is the tarball different per OS? Looking at release jobs it looks like it is only built once.
Sorry, something went wrong.
There was a problem hiding this comment.
I added runs for windows and linux.
Sorry, something went wrong.
There was a problem hiding this comment.
I am -1 on windows.
Sorry, something went wrong.
|
To my early comment, I guess i was wrong... it seems like the action is running even though the PR came from a fork https://github.com/nodejs/node/pull/32129/checks?check_run_id=490863314 I thought this was not able to happen based on how GitHub IAM works... |
Sorry, something went wrong.
|
@MylesBorins, it looks like my changes to the actions are running - eg you can see the job running here: https://github.com/nodejs/node/runs/490863314?check_suite_focus=true |
Sorry, something went wrong.
|
@jkleinsc it is only built once in ci-release, specifically on the centos7 machine, I believe. I'm more curious about potential platform issues when building from the source binary. For example there are code paths only for particular architectures / OSs, so would be a shame to miss coverage |
Sorry, something went wrong.
|
My suggestion would be to modify the existing build-* jobs to create the tar balls, extract them somewhere, then run the normal build. Only difficulty with that would be most of the GH action steps assume they're running from the GITHUB_WORKSPACE directory. The other non-build-* jobs probably need the full git repo contents, and should probably get split out of the single CI.yml with path filtering like #32143 in separate PRs |
Sorry, something went wrong.
There was a problem hiding this comment.
sgtm
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #32129 +/- ##
==========================================
- Coverage 97.29% 97.27% -0.02%
==========================================
Files 197 197
Lines 64971 65070 +99
==========================================
+ Hits 63215 63299 +84
- Misses 1756 1771 +15
Continue to review full report at Codecov.
|
Sorry, something went wrong.
|
@jkleinsc CI does some setup stuff to choose compiler you don't care about for just x64, and then does: # get ccache to reuse cache across workspaces
export CCACHE_BASEDIR=$(pwd)
if [ -z ${JOBS+x} ]; then
export JOBS=$(getconf _NPROCESSORS_ONLN)
fi
...
export NODE_TEST_DIR=${HOME}/node-tmp
export PYTHON=python
export FLAKY_TESTS=dontcare
make run-ci -j $JOBS V=1
|
Sorry, something went wrong.
There was a problem hiding this comment.
This part should consider removed, see #32458 (comment)
Sorry, something went wrong.
There was a problem hiding this comment.
This step is needed so that downstream build jobs have access to the tarball. I think this is a different case than #32458 (comment) because it isn't an executable. The other option would be to make the tarball on every platform but it doesn't appear that it can be made on Windows as far as I can tell. Also doing it that way would then make the jobs run longer as they would each have to build the tarball and then build from the tarball.
Sorry, something went wrong.
|
Please also consider split into a single file, see #32450. |
Sorry, something went wrong.
|
@jkleinsc This failed on Windows, afaict? |
Sorry, something went wrong.
|
@sam-github any insight into the Windows CI config? I can't get all the tests to pass there and it looks like maybe Jenkins splits the tests into multiple parallel runs? |
Sorry, something went wrong.
|
At least two of the failing tests (parallel/test-source-map-enable and wasi/test-wasi) appear to be failing due to a mismatch in line endings. There's a note in the README for tests saying autocrlf should be set to true in a git checkout on Windows for tests to pass: https://github.com/nodejs/node/tree/master/test#nodejs-core-tests |
Sorry, something went wrong.
|
@richardlau thanks for the hint. That did indeed resolve 2 of the failing tests. It looks like the test parallel/test-child-process-exec-any-shells-windows is still failing though. |
Sorry, something went wrong.
|
For Windows we'll need to add to PATH the path to some basic UNIX tools such as cat, head, sleep as per https://github.com/nodejs/node/blob/master/BUILDING.md#prerequisites
to fix test failures such as: not ok 39 async-hooks/test-graph.pipe
---
duration_ms: 0.93
severity: fail
exitcode: 1
stack: |-
events.js:292
throw er; // Unhandled 'error' event
^
Error: spawn sleep ENOENT
at Process.ChildProcess._handle.onexit (internal/child_process.js:268:19)
at onErrorNT (internal/child_process.js:468:16)
at processTicksAndRejections (internal/process/task_queues.js:84:21)
Emitted 'error' event on ChildProcess instance at:
at Process.ChildProcess._handle.onexit (internal/child_process.js:274:12)
at onErrorNT (internal/child_process.js:468:16)
at processTicksAndRejections (internal/process/task_queues.js:84:21) {
errno: -4058,
code: 'ENOENT',
syscall: 'spawn sleep',
path: 'sleep',
spawnargs: [ '0.1' ]
}
|
Sorry, something went wrong.
Git for Windows includes C:\Program Files\Git\bin\bash.exe which spawns ..\usr\bin\bash.exe so copying that executable won't work. However if a symlink is used to test paths with spaces this executable will still work.
Sorry, something went wrong.
|
@richardlau this is ready for review. I was able to sort out the Windows issues. |
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/30595/ (:heavy_check_mark:) |
Sorry, something went wrong.
Git for Windows includes `C:\Program Files\Git\bin\bash.exe`, which spawns ..\usr\bin\bash.exe, so copying that executable won't work. However, if a symlink is used to test paths with spaces, this executable will still work. PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Fixes: nodejs/build#1931 PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
|
Landed in 38bf1be...26924fa 🎉 Thanks for pushing this through, @jkleinsc! |
Sorry, something went wrong.
|
very nice work, this should be really valuable, thanks for putting up with a marathon @jkleinsc |
Sorry, something went wrong.
Git for Windows includes `C:\Program Files\Git\bin\bash.exe`, which spawns ..\usr\bin\bash.exe, so copying that executable won't work. However, if a symlink is used to test paths with spaces, this executable will still work. PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Fixes: nodejs/build#1931 PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Git for Windows includes `C:\Program Files\Git\bin\bash.exe`, which spawns ..\usr\bin\bash.exe, so copying that executable won't work. However, if a symlink is used to test paths with spaces, this executable will still work. PR-URL: nodejs#32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Git for Windows includes `C:\Program Files\Git\bin\bash.exe`, which spawns ..\usr\bin\bash.exe, so copying that executable won't work. However, if a symlink is used to test paths with spaces, this executable will still work. PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Fixes: nodejs/build#1931 PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Git for Windows includes `C:\Program Files\Git\bin\bash.exe`, which spawns ..\usr\bin\bash.exe, so copying that executable won't work. However, if a symlink is used to test paths with spaces, this executable will still work. PR-URL: #32129 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
| Back | FazBrowse Home | New Git URL |
This PR adds a GitHub action to build Node.js from a tarball.
Resolves: nodejs/build#1931
Checklist