Skip to content
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

build: add build from tarball #32129

Closed
wants to merge 4 commits into from

Conversation

jkleinsc
Copy link

@jkleinsc jkleinsc commented Mar 6, 2020

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

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

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added runs for windows and linux.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am -1 on windows.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor

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
Copy link
Author

jkleinsc commented Mar 6, 2020

@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
Copy link
Contributor

@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
Copy link
Member

nschonni commented Mar 8, 2020

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

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

@codecov-io
Copy link

codecov-io commented Mar 11, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

@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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should consider removed, see #32458 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@addaleax
Copy link
Member

@jkleinsc This failed on Windows, afaict?

@jkleinsc
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

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

@jkleinsc jkleinsc force-pushed the add-build-from-tarball branch 6 times, most recently from 19e6487 to d1fed7e Compare April 2, 2020 21:33
@addaleax addaleax requested a review from richardlau April 5, 2020 18:38
@addaleax 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
@richardlau
Copy link
Member

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

John Kleinschmidt added 3 commits April 9, 2020 11:12
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
Copy link
Collaborator

@jkleinsc
Copy link
Author

jkleinsc commented Apr 9, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 9, 2020

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

addaleax pushed a commit that referenced this pull request Apr 13, 2020
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>
addaleax pushed a commit that referenced this pull request Apr 13, 2020
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
Copy link
Member

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

@addaleax addaleax closed this Apr 13, 2020
@rvagg
Copy link
Member

rvagg commented Apr 14, 2020

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

BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
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>
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
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
Copy link
Contributor

see: #32848

@jkleinsc jkleinsc deleted the add-build-from-tarball branch April 14, 2020 16:15
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
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>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
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 BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check that tarballs can build