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

win, build: skip building cctest by default #21408

Closed
wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Jun 19, 2018

vcbuild will build cctest only if it will be run, or for CI and release builds.

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

/cc @nodejs/build-files @nodejs/platform-windows

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jun 19, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Jun 19, 2018

@BridgeAR
Copy link
Member

BridgeAR commented Jun 20, 2018

@nodejs/platform-windows PTAL

@seishun
Copy link
Contributor

seishun commented Jun 20, 2018

What's the logic behind building it for release builds?

@bzoz
Copy link
Contributor Author

bzoz commented Jun 20, 2018

@seishun: so that CI (which uses something like vcbuild release nosign) will also build cctest.exe

@@ -147,6 +149,7 @@ if defined build_release (
set licensertf=1
set download_arg="--download=all"
set i18n_arg=small-icu
set cctest=1
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK on the release CI (where build_release is used) tests aren't run. Anyone from @nodejs/releasers / @nodejs/build / @nodejs/platform-windows able to confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are probably not, but I guess someone somewhere might depend on that. This change will not make big change in terms of building release binaries, it is mostly aimed to help out when developing Node.

@bzoz
Copy link
Contributor Author

bzoz commented Jun 26, 2018

@bzoz
Copy link
Contributor Author

bzoz commented Jun 26, 2018

CI is green, can I have +1?

/cc @nodejs/platform-windows @nodejs/build-files

@bzoz bzoz force-pushed the bartek-cctest-when-needed branch from 66b12aa to 1115174 Compare June 27, 2018 09:05
@bzoz
Copy link
Contributor Author

bzoz commented Jun 28, 2018

ping?

@joyeecheung
Copy link
Member

I am a bit confused by the title, does this patch skip building cctest by default in normal dev workflows?

@bzoz
Copy link
Contributor Author

bzoz commented Jul 4, 2018

Yes. It will get built if you do vcbuild test though.

@joyeecheung
Copy link
Member

@bzoz Oh right I understand now, this moves cctest out of vcbuild that's not supposed to build tests.

@joyeecheung
Copy link
Member

@addaleax
Copy link
Member

ping @nodejs/platform-windows ?

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

Left two comments, otherwise looks good.

vcbuild.bat Show resolved Hide resolved
vcbuild.bat Outdated Show resolved Hide resolved
@lundibundi
Copy link
Member

@bzoz ping? =)

@bzoz bzoz force-pushed the bartek-cctest-when-needed branch from 1115174 to 7535c04 Compare August 28, 2018 11:59
@bzoz
Copy link
Contributor Author

bzoz commented Aug 28, 2018

Updated, but now it fails to build node from clean checkout:

LINK : fatal error LNK1104: cannot open file 'C:\...\node\Release\obj\global_intermediate\openssl.def' [C:\...\node\node.vcxproj]

if "%test_args%"=="" set target=rename_node_bin_win
if defined cctest set target="Build"
)
if "%target%"=="rename_node_bin_win" if exist "%config%\cctest.exe" del "%config%\cctest.exe"
Copy link
Member

Choose a reason for hiding this comment

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

First time seeing this file, so may be completely wrong, but: shouldn't you actually also assign target = node here?

Copy link
Member

Choose a reason for hiding this comment

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

@lundibundi not sure I understand your question, target might have just been changed above. This uses rename_node_bin_win instead of just node to be compatible with vcbuild dll, but should be the same otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah rename_node_bin_win is an old kludge.

node/node.gyp

Lines 834 to 842 in d1a55d3

{
# When using shared lib to build executable in Windows, in order to avoid
# filename collision, the executable name is node-win.exe. Need to rename
# it back to node.exe
'target_name': 'rename_node_bin_win',
'type': 'none',
'dependencies': [
'<(node_core_target_name)',
],

I have a refactor for this in the pipeline.

@lundibundi
Copy link
Member

ping @bzoz.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Oct 17, 2018
@bzoz bzoz removed the stalled Issues and PRs that are stalled. label Oct 24, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Oct 24, 2018

Updated, PTAL.

It looks like the linking issue got resolved, the codebase builds correctly after clean checkout.

@bzoz
Copy link
Contributor Author

bzoz commented Oct 24, 2018

@bzoz
Copy link
Contributor Author

bzoz commented Oct 25, 2018

Resumed with fixed vcbuild: https://ci.nodejs.org/job/node-test-pull-request/18138/

vcbuild.bat Outdated
if "%target%"=="Build" if defined no_cctest set target=node
if "%target%"=="Build" (
if defined no_cctest set target=rename_node_bin_win
if "%test_args%"=="" set target=rename_node_bin_win
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if "%test_args%"=="" set target=rename_node_bin_win
if "%test_args%"=="" set target=rename_node_bin_win

nit: trailing whitespace

set openssl_no_asm=
set doc=

:next-arg
if "%1"=="" goto args-done
if /i "%1"=="debug" set config=Debug&goto arg-ok
if /i "%1"=="release" set config=Release&set ltcg=1&set "pch="&goto arg-ok
if /i "%1"=="release" set config=Release&set ltcg=1&set "pch="&set cctest=1&goto arg-ok
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should create a ci target, since the "real" release doesn't need cctest either.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM, this should converge to be like the Makefile, including the distinction between compiling and js-only targets. Currently cctest runs and the addons are compiled everywhere, unnecessarily.

vcbuild will build cctest only if it will be run, or for CI and
release builds
@bzoz
Copy link
Contributor Author

bzoz commented Nov 22, 2018

Fixed whitespace and reabased, PTAL

@bzoz
Copy link
Contributor Author

bzoz commented Nov 22, 2018

looks like Travis is having issues, cc @nodejs/build-infra ?

@rvagg
Copy link
Member

rvagg commented Nov 22, 2018

https://travis-ci.com/nodejs/node/jobs/160294272

"message": "API rate limit exceeded for 35.192.187.174. (But here'''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
"documentation_url": "https://developer.github.com/v3/#rate-limiting"

@bzoz Ignore the error for now, it's clear that your commit message is fine.

We only get 60 requests per hour unauthenticated.. I suppose we'll need to get authentication into that script.

@richardlau @addaleax thoughts on where we should log an issue about this? Is it something we should take on in Build since it's going to require credentials and inclusion of secrets?

@rvagg
Copy link
Member

rvagg commented Nov 22, 2018

Oh, and we're sharing rate limiting with everyone else using these Travis hosts, so we're bound to run into this with some regularity. I'm surprised it hasn't come up before; maybe it has?

@richardlau
Copy link
Member

Oh, and we're sharing rate limiting with everyone else using these Travis hosts, so we're bound to run into this with some regularity. I'm surprised it hasn't come up before; maybe it has?

Yes, it has. I've opened #24567 to track separately.

@bzoz
Copy link
Contributor Author

bzoz commented Nov 22, 2018

@bzoz
Copy link
Contributor Author

bzoz commented Nov 29, 2018

Landed in 2c473d6

@bzoz bzoz closed this Nov 29, 2018
bzoz added a commit that referenced this pull request Nov 29, 2018
vcbuild will build cctest only if it will be run, or for CI and
release builds

PR-URL: #21408
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Nov 29, 2018
vcbuild will build cctest only if it will be run, or for CI and
release builds

PR-URL: #21408
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
vcbuild will build cctest only if it will be run, or for CI and
release builds

PR-URL: nodejs#21408
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
vcbuild will build cctest only if it will be run, or for CI and
release builds

PR-URL: #21408
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
vcbuild will build cctest only if it will be run, or for CI and
release builds

PR-URL: #21408
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Refael Ackermann <[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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.