build: add build from tarball #32129

jkleinsc · 2020-03-06T17:56:01Z

This PR adds a GitHub action to build Node.js from a tarball.

Resolves: nodejs/build#1931

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

MylesBorins

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?

MylesBorins · 2020-03-06T18:42:30Z

.github/workflows/CI.yml

@@ -39,6 +39,25 @@ jobs:
        run: npx envinfo
      - name: Build
        run: ./configure && make -j8
+  build-from-tarball:


do we want to run this on windows / linux too?

@MylesBorins is the tarball different per OS? Looking at release jobs it looks like it is only built once.

I added runs for windows and linux.

I am -1 on windows.

.github/workflows/CI.yml

MylesBorins · 2020-03-06T18:45:21Z

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...

jkleinsc · 2020-03-06T18:48:15Z

@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

MylesBorins · 2020-03-06T18:53:42Z

@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

nschonni · 2020-03-08T04:11:55Z

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

rvagg

sgtm

.github/workflows/CI.yml

codecov-io · 2020-03-11T03:16:52Z

Codecov Report

Merging #32129 into master will decrease coverage by 0.01%.
The diff coverage is 97.19%.

@@            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

Impacted Files	Coverage Δ
lib/internal/net.js	`100.00% <ø> (ø)`
lib/internal/streams/destroy.js	`98.87% <ø> (-0.01%)`	⬇️
lib/_http_outgoing.js	`95.54% <71.42%> (-0.30%)`	⬇️
lib/async_hooks.js	`99.36% <86.66%> (-0.64%)`	⬇️
lib/wasi.js	`98.49% <94.11%> (-1.51%)`	⬇️
lib/fs.js	`96.69% <95.23%> (-0.17%)`	⬇️
lib/_http_client.js	`98.38% <97.56%> (-0.19%)`	⬇️
lib/_http_agent.js	`99.10% <100.00%> (+0.03%)`	⬆️
lib/_stream_readable.js	`98.54% <100.00%> (ø)`
lib/_stream_transform.js	`100.00% <100.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 434d39d...e3b107b. Read the comment docs.

sam-github · 2020-03-24T19:11:34Z

@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

gengjiawen · 2020-03-26T00:41:11Z

.github/workflows/CI.yml

+          ./configure && make tar -j8
+          mkdir tarballs
+          mv *.tar.gz tarballs
+      - name: Upload tarball artifact


This part should consider removed, see #32458 (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.

gengjiawen · 2020-03-26T01:43:23Z

Please also consider split into a single file, see #32450.

addaleax · 2020-03-29T20:11:35Z

@jkleinsc This failed on Windows, afaict?

jkleinsc · 2020-03-30T17:10:46Z

@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?

richardlau · 2020-03-30T17:51:35Z

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

jkleinsc · 2020-03-30T20:55:04Z

@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.

richardlau · 2020-04-06T10:34:32Z

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

Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH.

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' ]
    }

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.

nodejs-github-bot · 2020-04-09T16:52:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30594/

jkleinsc · 2020-04-09T17:44:54Z

@richardlau this is ready for review. I was able to sort out the Windows issues.

nodejs-github-bot · 2020-04-09T17:49:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30595/ (:heavy_check_mark:)

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>

addaleax · 2020-04-13T16:36:00Z

Landed in 38bf1be...26924fa 🎉 Thanks for pushing this through, @jkleinsc!

rvagg · 2020-04-14T00:53:28Z

very nice work, this should be really valuable, thanks for putting up with a marathon @jkleinsc

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>

sam-github · 2020-04-14T16:13:51Z

see: #32848

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>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Mar 6, 2020

jkleinsc mentioned this pull request Mar 6, 2020

Check that tarballs can build nodejs/build#1931

Closed

MylesBorins reviewed Mar 6, 2020

View reviewed changes

rvagg approved these changes Mar 9, 2020

View reviewed changes

richardlau reviewed Mar 11, 2020

View reviewed changes

.github/workflows/CI.yml Outdated Show resolved Hide resolved

jkleinsc force-pushed the add-build-from-tarball branch from ea76457 to d902a32 Compare March 18, 2020 14:03

jkleinsc mentioned this pull request Mar 24, 2020

Introduction: I'm John. How can I help? nodejs/build#2197

Closed

jkleinsc force-pushed the add-build-from-tarball branch from f89f986 to 5f4628e Compare March 26, 2020 00:07

gengjiawen reviewed Mar 26, 2020

View reviewed changes

jkleinsc force-pushed the add-build-from-tarball branch 6 times, most recently from 19e6487 to d1fed7e Compare April 2, 2020 21:33

addaleax approved these changes Apr 5, 2020

View reviewed changes

addaleax requested a review from richardlau April 5, 2020 18:38

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 5, 2020

John Kleinschmidt added 3 commits April 9, 2020 11:12

test: use symlinks to copy shells

bbdf382

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.

Use Python 2.7 for Windows testing

67d4d2b

Move built release to checkout dir

f5dc4d2

jkleinsc force-pushed the add-build-from-tarball branch from 1cb0c4d to f5dc4d2 Compare April 9, 2020 15:12

richardlau approved these changes Apr 9, 2020

View reviewed changes

addaleax pushed a commit that referenced this pull request Apr 13, 2020

build: add build from tarball

26924fa

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>

addaleax closed this Apr 13, 2020

BethGriggs pushed a commit that referenced this pull request Apr 14, 2020

build: add build from tarball

4e5ec41

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>

BethGriggs mentioned this pull request Apr 14, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

sam-github mentioned this pull request Apr 14, 2020

"build from tarball" github action is flaky? #32848

Closed

jkleinsc deleted the add-build-from-tarball branch April 14, 2020 16:15

targos added dont-land-on-v10.x labels Apr 25, 2020

BridgeAR pushed a commit that referenced this pull request Apr 28, 2020

build: add build from tarball

c6078f0

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>

BridgeAR mentioned this pull request Apr 28, 2020

v13.14.0 proposal #33122

Merged

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add build from tarball #32129

build: add build from tarball #32129

jkleinsc commented Mar 6, 2020 •

edited

MylesBorins left a comment

MylesBorins Mar 6, 2020

jkleinsc Mar 6, 2020

jkleinsc Mar 6, 2020

gengjiawen Mar 11, 2020

MylesBorins commented Mar 6, 2020

jkleinsc commented Mar 6, 2020

MylesBorins commented Mar 6, 2020

nschonni commented Mar 8, 2020

rvagg left a comment

codecov-io commented Mar 11, 2020 •

edited

sam-github commented Mar 24, 2020

gengjiawen Mar 26, 2020

jkleinsc Mar 26, 2020

gengjiawen commented Mar 26, 2020

addaleax commented Mar 29, 2020

jkleinsc commented Mar 30, 2020

richardlau commented Mar 30, 2020

jkleinsc commented Mar 30, 2020

richardlau commented Apr 6, 2020

nodejs-github-bot commented Apr 9, 2020

jkleinsc commented Apr 9, 2020

nodejs-github-bot commented Apr 9, 2020 •

edited by addaleax

addaleax commented Apr 13, 2020

rvagg commented Apr 14, 2020

sam-github commented Apr 14, 2020

build: add build from tarball #32129

build: add build from tarball #32129

Conversation

jkleinsc commented Mar 6, 2020 • edited

Checklist

MylesBorins left a comment

Choose a reason for hiding this comment

MylesBorins Mar 6, 2020

Choose a reason for hiding this comment

jkleinsc Mar 6, 2020

Choose a reason for hiding this comment

jkleinsc Mar 6, 2020

Choose a reason for hiding this comment

gengjiawen Mar 11, 2020

Choose a reason for hiding this comment

MylesBorins commented Mar 6, 2020

jkleinsc commented Mar 6, 2020

MylesBorins commented Mar 6, 2020

nschonni commented Mar 8, 2020

rvagg left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 11, 2020 • edited

Codecov Report

sam-github commented Mar 24, 2020

gengjiawen Mar 26, 2020

Choose a reason for hiding this comment

jkleinsc Mar 26, 2020

Choose a reason for hiding this comment

gengjiawen commented Mar 26, 2020

addaleax commented Mar 29, 2020

jkleinsc commented Mar 30, 2020

richardlau commented Mar 30, 2020

jkleinsc commented Mar 30, 2020

richardlau commented Apr 6, 2020

nodejs-github-bot commented Apr 9, 2020

jkleinsc commented Apr 9, 2020

nodejs-github-bot commented Apr 9, 2020 • edited by addaleax

addaleax commented Apr 13, 2020

rvagg commented Apr 14, 2020

sam-github commented Apr 14, 2020

jkleinsc commented Mar 6, 2020 •

edited

codecov-io commented Mar 11, 2020 •

edited

nodejs-github-bot commented Apr 9, 2020 •

edited by addaleax