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, windows: build and test add-ons on test-ci #5886

Closed
wants to merge 6 commits into from
Closed

build, windows: build and test add-ons on test-ci #5886

wants to merge 6 commits into from

Conversation

blobor
Copy link

@blobor blobor commented Mar 24, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

Affected core subsystem(s)

build on Windows OS

Description of change

added build-addons task, it allows to build and test native addons during test-ci task.
Basicaly it should work in same way like Makefile "build-addons" task.

Fixes: #2537

added build-addons task, it allows to build and test native addons during test-ci task.
Basicaly it should work in same way like Makefile "build-addons" task.
@mscdex mscdex added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. labels Mar 24, 2016
@rvagg
Copy link
Member

rvagg commented Mar 24, 2016

ok then https://ci.nodejs.org/job/node-test-pull-request/2053/

lgtm tho this is stretching my .bat skillz

/cc @nodejs/platform-windows

@blobor
Copy link
Author

blobor commented Mar 24, 2016

looks like build is failing

@rvagg
Copy link
Member

rvagg commented Mar 24, 2016

oh right, that's because we build binaries on one server and fan the tests across a number of non-build servers, so it's going to be a bit more complicated than just activating this in test-ci, @joaocgreis over to you to weigh in on the feasability of this, do we do this for the rpi fanned tests already?

@blobor
Copy link
Author

blobor commented Mar 24, 2016

@rvagg I saw in build logs:

c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vcbt2015\label\win10>tar xavf binary/binary.tar.gz 
config.gypi
icu_config.gypi
Release/node.exe
Release/openssl-cli.exe
Release/cctest.exe

So, before we start running tests, we unpacking binaries.
Basically, I just need to move "node_exe" variable assignment on the top. And this should fix this issue.

@blobor
Copy link
Author

blobor commented Mar 24, 2016

Build could fail on "load-long-path" test due to: #3667

this should fix issue with running test-ci on non-build servers.
@blobor
Copy link
Author

blobor commented Mar 24, 2016

Hey @rvagg, could you please try tun CI build again?

@joaocgreis
Copy link
Member

Hi @blobor , thanks for contributing!

In the CI server we test Windows and Raspberry Pis with some added logic, because we compile and run tests in different machines. The Pis are already running addons tests, the test runners execute make test/addons/.buildstamp before calling test.py (...) addons directly, without using make. However, windows still uses test-ci. You can simulate the current commands locally with:

vcbuild.bat release nosign x64
tar cavf ../binary.tar.gz config.gypi icu_config.gypi Release/node.exe Release/openssl-cli.exe Release/cctest.exe
git clean -fdx
tar xavf ../binary.tar.gz
vcbuild.bat release nosign x64 nobuild test-ci ignore-flaky

(we use the tar bundled with Git for Windows, but it doesn't have to be tar, just have to save those exact files and delete everything else)

We can start saving node.lib if it is needed.

I think this would be great if you could keep the build_addons variable, but instead of adding a if /i "%1"=="build-addons", add a if /i "%1"=="test-addons" that sets build_addons and only tests addons? addons also needs to be added to test.

Renamed build-addons -> test-addons
test-addons task shouldtest only addons
test should task also test addons
@blobor
Copy link
Author

blobor commented Mar 24, 2016

Hey @joaocgreis, thanks for your reply.

I've made changes accordingly to your comment. Could you please review them?

set build_addons=

:: assign path to node_exe
set node_exe="%config%\node.exe"
Copy link
Member

Choose a reason for hiding this comment

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

The correct place for this is after all set config= have happened, before the configure_flags are prepared, exactly at

. @blobor can you move this?

Copy link
Member

Choose a reason for hiding this comment

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

The quotes should not be stored in the variable, but added to every place where the variable is used, to be consistent with all other variables in this file.

@joaocgreis
Copy link
Member

@blobor I did a careful review, left some comments. Can you address them? Thanks!

I've updated the CI job to pack node.lib, so next time I expect it to pass.

Current status is:

  • release x64: builds and tests, all ok
  • release x86: fails load-long-path, as @blobor mentioned above
  • debug: fails all but repl-domain-abort, as it does in Unix

I believe this should land with the release x86 failure. That configuration is not tested in CI, so it will not block. That failure is not a consequence of this and is being tracked at #3667 , so this might even be useful in fixing it.

move "node_exe" variable assignment
change backslashes
@blobor
Copy link
Author

blobor commented Mar 28, 2016

@joaocgreis, thanks for review. I've made those changes.

@joaocgreis
Copy link
Member

LGTM

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

Will land in about 24h if CI is ok and there are no objections.

@blobor
Copy link
Author

blobor commented Mar 28, 2016

Looks like CI failed again on win 2008 VS2013 machine.

@joaocgreis
Copy link
Member

Looks like a problem with Windows SDK on that machine, I've seen it on others. I can't do it right now, but I'll fix it and re-run CI. Everything else passed, so I don't expect a problem there.

@blobor
Copy link
Author

blobor commented Mar 28, 2016

@joaocgreis, great! Thank you so much for your help!

joaocgreis pushed a commit to JaneaSystems/node that referenced this pull request Mar 29, 2016
Added build-addons task, it allows to build and test native addons
during test-ci task. Basicaly it should work in same way like Makefile
"build-addons" task.

PR-URL: nodejs#5886
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
joaocgreis pushed a commit that referenced this pull request Mar 29, 2016
Added build-addons task, it allows to build and test native addons
during test-ci task. Basically it should work in same way like
Makefile "build-addons" task.

Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #5886
Fixes: #2537
@joaocgreis
Copy link
Member

CI passed this time: https://ci.nodejs.org/job/node-test-commit/2723/

Landed in: c7138e9

Thank you @blobor !

@blobor blobor closed this Mar 29, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Added build-addons task, it allows to build and test native addons
during test-ci task. Basically it should work in same way like
Makefile "build-addons" task.

Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #5886
Fixes: #2537
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Added build-addons task, it allows to build and test native addons
during test-ci task. Basically it should work in same way like
Makefile "build-addons" task.

Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #5886
Fixes: #2537
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Added build-addons task, it allows to build and test native addons
during test-ci task. Basically it should work in same way like
Makefile "build-addons" task.

Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #5886
Fixes: #2537
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
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. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants