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: for --enable-static, run only cctest #14892

Closed
wants to merge 3 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 17, 2017

Refs: #14158

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 17, 2017
@danbev
Copy link
Contributor Author

danbev commented Aug 17, 2017

@mscdex
Copy link
Contributor

mscdex commented Aug 17, 2017

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')?

@refack
Copy link
Contributor

refack commented Aug 17, 2017

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.

@refack refack added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 17, 2017
@danbev
Copy link
Contributor Author

danbev commented Aug 18, 2017

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.

@danbev danbev changed the title build: only run cctest when --enable-static build: for --enable-static, run only cctest Aug 18, 2017
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.
@danbev
Copy link
Contributor Author

danbev commented Aug 21, 2017

Running .\vcbuild.bat test static should now only run the cctest target.
@danbev
Copy link
Contributor Author

danbev commented Aug 22, 2017

@danbev
Copy link
Contributor Author

danbev commented Aug 22, 2017

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

@danbev
Copy link
Contributor Author

danbev commented Aug 22, 2017

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

@refack
Copy link
Contributor

refack commented Aug 22, 2017

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

@@ -192,6 +192,10 @@ v8:
tools/make-v8.sh
$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)

ifeq ($(NODE_TARGET_TYPE),static_library)
test: all
$(MAKE) cctest
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@lance
Copy link
Member

lance commented Aug 23, 2017

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?

@danbev
Copy link
Contributor Author

danbev commented Aug 23, 2017

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

@danbev
Copy link
Contributor Author

danbev commented Aug 23, 2017

@refack Thanks for the windows update!

@danbev
Copy link
Contributor Author

danbev commented Aug 23, 2017

@lance lance self-requested a review August 23, 2017 12:39
@refack
Copy link
Contributor

refack commented Aug 23, 2017

Cross-ref: #14986

danbev added a commit to danbev/node that referenced this pull request Aug 24, 2017
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 <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Aug 24, 2017

Landed in a36b540

@danbev danbev closed this Aug 24, 2017
@danbev danbev deleted the skip-test-enable-static branch August 24, 2017 05:09
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
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 <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
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 <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
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 <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
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 <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
@MylesBorins
Copy link
Contributor

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

danbev added a commit to danbev/node that referenced this pull request Nov 16, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
danbev added a commit to danbev/node that referenced this pull request Nov 16, 2017
This reverts commit a36b540.

PR-URL: nodejs#14986
Refs: nodejs#14158
Refs: nodejs#14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
This reverts commit a36b540.

PR-URL: #14986
Refs: #14158
Refs: #14892
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants