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

jenkins: add overlapped-checker.exe to binary.tgz #2495

Merged
merged 1 commit into from
Dec 13, 2020

Conversation

tarruda
Copy link
Contributor

@tarruda tarruda commented Dec 8, 2020

Required for testing overlapped I/O on windows (#29412)

@aduh95
Copy link
Contributor

aduh95 commented Dec 8, 2020

Needed for nodejs/node#29412.

@rvagg rvagg requested a review from joaocgreis December 9, 2020 03:54
@tarruda
Copy link
Contributor Author

tarruda commented Dec 9, 2020

I'm not sure if this fixes the build for nodejs/node#29412. I've added a temporary commit there which changes the reference to my fork of nodejs/build, but tt seems someone with access needs to add the request-ci label to that PR in order for all tests to be executed.

@@ -15,7 +15,7 @@ call %~dp0compile.cmd
if errorlevel 1 exit /b

:: Select files to pack
set "BINARY_FILES=config.gypi icu_config.gypi Release/node.exe Release/node.lib Release/openssl-cli.exe Release/cctest.exe Release/node.pdb"
set "BINARY_FILES=config.gypi icu_config.gypi Release/node.exe Release/node.lib Release/openssl-cli.exe Release/cctest.exe Release/node.pdb Release/overlapped-checker.exe"
if exist Release\node_test_engine.dll set "BINARY_FILES=%BINARY_FILES% Release/node_test_engine.dll"
Copy link
Member

Choose a reason for hiding this comment

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

The file should be added in a line like this instead, or it will break CI when the file is not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Required for testing overlapped I/O on windows (#29412)
@joaocgreis joaocgreis merged commit 42128d5 into nodejs:master Dec 13, 2020
@joaocgreis
Copy link
Member

Thanks!

Landed and tested, LTS versions had no issues but master seems to be broken. It's unrelated to these changes though (https://ci.nodejs.org/view/All/job/node-test-commit-windows-fanned/39968/ vs https://ci.nodejs.org/view/All/job/node-test-commit-windows-fanned/39966/).

@tarruda
Copy link
Contributor Author

tarruda commented Dec 13, 2020

@joaocgreis thanks for merging.

Unfortunately it seems nodejs/node#29412 is still failing (with the same error, overlapped-checker cannot be found by the test that uses it).

The problem seems to be that some CI runs don't build node.js but actually download a tarball with binaries (I noticed that some linux tests also do that). Do you have any idea how we can include overlapped-checker in the generated tarball?

@joaocgreis
Copy link
Member

When using the "Resume build" button in Jenkins, it tries to use the results from old jobs that passed. This is very useful in some situations, but in this case we need a proper full Rebuild: https://ci.nodejs.org/job/node-test-pull-request/34930/

The arm-fanned (Raspberry PI) job does not use configuration stored here like the Windows job. The commands are still in Jenkins config. I edited and added the file there. Let's see if I got it right.

@tarruda
Copy link
Contributor Author

tarruda commented Dec 14, 2020

@joaocgreis It seems your jenkins config change worked (the arm build passed), now only windows is still failing: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/7243/

I've checked the failing child jobs under node-test-binary-windows-js-suites (win10-COMPILED_BY-vs2019/win2012r2-COMPILED_BY-vs2019-x86), and both are failing with the same error:

Error: spawn C:\workspace\node-test-binary-windows-js-suites\node\out\Release\overlapped-checker.exe ENOENT

Any clues on why overlapped-checker.exe is still not being found?

@joaocgreis
Copy link
Member

@tarruda I suspect it is because of using out/Release/ instead of Release/. It used to be only Release/, which is what CI is using, but changed to include out and vcbuild creates a link during build, which might not happen when running the tests in CI. Perhaps vcbuild should create the link in all cases, but I'm not able to look into that at the moment. To move forward, if I'm right about this, the easiest way is to find overlapped-checker relative to the executable, like what is done for openssl-cli in https://github.com/nodejs/node/blob/a1509261770cb64527e55645503e0d9b9d61ae87/test/common/index.js#L837-L840 .

AshCripps pushed a commit that referenced this pull request Jan 8, 2021
Required for testing overlapped I/O on windows (nodejs/node#29412)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants