build: for --enable-static, run only cctest by danbev · Pull Request #14892 · nodejs/node · GitHub

Currently when building with --enable-static and running the test target
the following error will be reported:

Building addon
/node/test/addons/01_function_arguments/
env: ./node: No such file or directory
make[1]: *** [test/addons/.buildstamp] Error 1

Note that this is with a clean build where no prior node executable was
built.

This commit suggests only running the cctest target when --enable-static
is specified.

[refack: added refs]

Checklist

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

Affected core subsystem(s)

build

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

The commit message wording is a little confusing, it makes it sound as if the only time cctest is run is when --enable-static is used. Perhaps swapping the order may make it clearer (e.g. 'build: for --enable-static, run only cctest')?

LGTM until we have a harness to test the static/dynamic library targets.
@danbev nice to have: same change in vcbuild.bat but not a blocker for me.

The commit message wording is a little confusing, it makes it sound as if the only time cctest is run is when --enable-static is used. Perhaps swapping the order may make it clearer (e.g. 'build: for --enable-static, run only cctest')?

Yesterday was not a good day :( I agree and reading it now it is confusing. I like your suggestion and I'll update now. Thanks.

Currently when building with --enable-static and running the test target the following error will be reported: Building addon /node/test/addons/01_function_arguments/ env: ./node: No such file or directory make[1]: *** [test/addons/.buildstamp] Error 1 Note that this is with a clean build where no prior node executable was built. This commit suggests only running the cctest target when --enable-static is specified.

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

Running .\vcbuild.bat test static should now only run the cctest target.

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

test/osx failure looks unrelated

console output:

Triggering node-test-commit-osx » osx1010
Configuration node-test-commit-osx » osx1010 is still in the queue: Waiting for next available executor on test-requireio-osx1010-x64-1
node-test-commit-osx » osx1010 completed with result FAILURE
Notifying upstream projects of job completion
Finished: FAILURE

test/arm failure looks unrelated

console output:

not ok 796 parallel/test-http2-multiplex
  ---
  duration_ms: 1.425
  severity: fail
  stack: |-
    (node:28491) ExperimentalWarning: The http2 module is an experimental API.
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'abcdefjj' === 'abcdefghij'
        at ClientHttp2Stream.req.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/test/parallel/test-http2-multiplex.js:41:14)
        at ClientHttp2Stream.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/test/common/index.js:509:15)
        at emitNone (events.js:110:20)
        at ClientHttp2Stream.emit (events.js:207:7)
        at endReadableNT (_stream_readable.js:1059:12)
        at _combinedTickCallback (internal/process/next_tick.js:138:11)
        at process._tickCallback (internal/process/next_tick.js:180:9)
  ...

@refack Would you be able to take a look at the windows related update for this?

@refack Would you be able to take a look at the windows related update for this?

Thanks for remembering vcbuild.bat!

cmd's if is not intuitive in the way it deals with multiple statements in the then, so I suggest to flip it (if defined X "skip").
I pushed a suggestion commit with the a change that I've tested. Obviously feel free to push it out if you don't like it.

Should $(MAKE) lint not also be run as a part of the static build?

I'm +0 on that.
It is already run by the patched vcbuild.bat though.

No, wait that's a problem, if there's no node binary that will fail.

Ah now I see. Good point. 👍

~/s/node ❯❯❯ make lint                                                                                  master ⬆ ⬇ ✭ ◼ │
Running JS linter...                                                                                                   │
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.md \                                │
          benchmark doc lib test tools                                                                                 │
make[1]: ./node: No such file or directory                                                                             │
make[1]: *** [jslint] Error 1

Just so I understand... when building with --enable-static, no node binary is produced, and so therefore no node tests are run. So if I'm a developer hacking on Node core for my custom embedded node, my workflow would need to be such that when I make changes to Node core, I will need to first run make -j4 test, to build a node binary to test against, and then run make --enable-static to produce my static library?

Really just curious here, but how difficult would it be (and is it really desirable) to have a test harness that, when building with --enable-static loads the static library and feeds the tests through that. Or is this just a lot of work for negligible gain?

@lance Yeah, that is my understanding of the workflow at the moment.

Really just curious here, but how difficult would it be (and is it really desirable) to have a test harness that, when building with --enable-static loads the static library and feeds the tests through that. Or is this just a lot of work for negligible gain?

This idea came up earlier where we thought of doing something like this for the static lib and also when building a shared library. I have a done some work on this and hope to follow this up with a suggestion. I need to sort out a couple of issue I ran into. Hopefully the work on --enable-static we done to this point at least improves things a little.

@refack Thanks for the windows update!

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

Cross-ref: #14986

Currently when building with --enable-static and running the test target the following error will be reported: Building addon /node/test/addons/01_function_arguments/ env: ./node: No such file or directory make[1]: *** [test/addons/.buildstamp] Error 1 Note that this is with a clean build where no prior node executable was built. This commit suggests only running the cctest target when --enable-static is specified. PR-URL: nodejs#14892 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com>

Landed in a36b540

Currently when building with --enable-static and running the test target the following error will be reported: Building addon /node/test/addons/01_function_arguments/ env: ./node: No such file or directory make[1]: *** [test/addons/.buildstamp] Error 1 Note that this is with a clean build where no prior node executable was built. This commit suggests only running the cctest target when --enable-static is specified. PR-URL: nodejs/node#14892 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com>

Currently when building with --enable-static and running the test target the following error will be reported: Building addon /node/test/addons/01_function_arguments/ env: ./node: No such file or directory make[1]: *** [test/addons/.buildstamp] Error 1 Note that this is with a clean build where no prior node executable was built. This commit suggests only running the cctest target when --enable-static is specified. PR-URL: #14892 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com>

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

The motivation for this commit is to enable projects embedding Node.js and building with --enable-static to be able to run the test suite and linter. Currently when building with --enable-static no node executable will be created which means that the tests (apart from the cctest) and linter cannot be run. This is currently a work in progress and works on MacOS but I need to run the CI, and manually on different environments to verify that it works as expected. PR-URL: nodejs#14986 Refs: nodejs#14158 Refs: nodejs#14892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This reverts commit a36b540. PR-URL: nodejs#14986 Refs: nodejs#14158 Refs: nodejs#14892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

The motivation for this commit is to enable projects embedding Node.js and building with --enable-static to be able to run the test suite and linter. Currently when building with --enable-static no node executable will be created which means that the tests (apart from the cctest) and linter cannot be run. This is currently a work in progress and works on MacOS but I need to run the CI, and manually on different environments to verify that it works as expected. PR-URL: #14986 Refs: #14158 Refs: #14892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This reverts commit a36b540. PR-URL: #14986 Refs: #14158 Refs: #14892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

The motivation for this commit is to enable projects embedding Node.js and building with --enable-static to be able to run the test suite and linter. Currently when building with --enable-static no node executable will be created which means that the tests (apart from the cctest) and linter cannot be run. This is currently a work in progress and works on MacOS but I need to run the CI, and manually on different environments to verify that it works as expected. PR-URL: #14986 Refs: #14158 Refs: #14892 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 17, 2017

refack approved these changes Aug 17, 2017

View reviewed changes

refack added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 17, 2017

danbev force-pushed the skip-test-enable-static branch from 74b963d to 84c7bf3 Compare August 18, 2017 06:08

danbev changed the title build: only run cctest when --enable-static build: for --enable-static, run only cctest Aug 18, 2017

danbev force-pushed the skip-test-enable-static branch from 84c7bf3 to 511f090 Compare August 21, 2017 10:03

squash: same logic for windows build

c9ef989

Running .\vcbuild.bat test static should now only run the cctest target.

danbev force-pushed the skip-test-enable-static branch from 511f090 to c9ef989 Compare August 22, 2017 08:42

lance reviewed Aug 22, 2017

View reviewed changes

[suggestion] flipped if and skip jslint

a159bfa

refack force-pushed the skip-test-enable-static branch from ee63d2b to a159bfa Compare August 22, 2017 21:37

lance self-requested a review August 23, 2017 12:39

lance approved these changes Aug 23, 2017

View reviewed changes

danbev mentioned this pull request Aug 23, 2017

test: --enable-static linked executable #14986

Closed

2 tasks

danbev closed this Aug 24, 2017

danbev deleted the skip-test-enable-static branch August 24, 2017 05:09

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

build: for --enable-static, run only cctest by danbev · Pull Request #14892 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: for --enable-static, run only cctest - #14892

build: for --enable-static, run only cctest#14892danbev wants to merge 3 commits intonodejs:masternodejs/node:masterfrom danbev:skip-test-enable-staticdanbev/node:skip-test-enable-staticCopy head branch name to clipboard

Conversation

danbev commented Aug 17, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Aug 17, 2017

Uh oh!

mscdex commented Aug 17, 2017

Uh oh!

refack commented Aug 17, 2017

Uh oh!

danbev commented Aug 18, 2017

Uh oh!

danbev commented Aug 21, 2017

Uh oh!

danbev commented Aug 22, 2017

Uh oh!

danbev commented Aug 22, 2017

Uh oh!

danbev commented Aug 22, 2017

Uh oh!

refack commented Aug 22, 2017

Uh oh!

lance Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

lance Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

lance commented Aug 23, 2017

Uh oh!

danbev commented Aug 23, 2017

Uh oh!

danbev commented Aug 23, 2017

Uh oh!

danbev commented Aug 23, 2017

Uh oh!

refack commented Aug 23, 2017

Uh oh!

danbev commented Aug 24, 2017

Uh oh!

MylesBorins commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

build: for --enable-static, run only cctest#14892
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:skip-test-enable-static

danbev commented Aug 17, 2017 •
edited by refack
Loading