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.

Sign up for GitHub

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

Jump to bottom

win, build: skip building cctest by default #21408

Closed
wants to merge 1 commit into from
Closed

win, build: skip building cctest by default #21408

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,14 @@ set http2_debug=
set nghttp2_debug=
set link_module=
set no_cctest=
set cctest=
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.

if /i "%1"=="clean" set target=Clean&goto arg-ok
if /i "%1"=="ia32" set target_arch=x86&goto arg-ok
if /i "%1"=="x86" set target_arch=x86&goto arg-ok
Expand Down Expand Up @@ -125,6 +126,7 @@ if /i "%1"=="no-NODE-OPTIONS" set no_NODE_OPTIONS=1&goto arg-ok
if /i "%1"=="debug-nghttp2" set debug_nghttp2=1&goto arg-ok
if /i "%1"=="link-module" set "link_module= --link-module=%2%link_module%"&goto arg-ok-2
if /i "%1"=="no-cctest" set no_cctest=1&goto arg-ok
if /i "%1"=="cctest" set cctest=1&goto arg-ok
if /i "%1"=="openssl-no-asm" set openssl_no_asm=1&goto arg-ok
if /i "%1"=="doc" set doc=1&goto arg-ok

Expand All @@ -151,6 +153,7 @@ if defined build_release (
set download_arg="--download=all"
set i18n_arg=small-icu
set projgen=1
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.

set ltcg=1
set "pch="
)
Expand Down Expand Up @@ -301,7 +304,12 @@ set "msbcpu=/m:2"
if "%NUMBER_OF_PROCESSORS%"=="1" set "msbcpu=/m:1"
set "msbplatform=Win32"
if "%target_arch%"=="x64" set "msbplatform=x64"
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
if defined cctest set target="Build"
)
joaocgreis marked this conversation as resolved.
Show resolved Hide resolved
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.

msbuild node.sln %msbcpu% /t:%target% /p:Configuration=%config% /p:Platform=%msbplatform% /clp:NoSummary;NoItemAndPropertyList;Verbosity=minimal /nologo
if errorlevel 1 (
if not defined project_generated echo Building Node with reused solution failed. To regenerate project files use "vcbuild projgen"
Expand Down Expand Up @@ -533,7 +541,7 @@ if "%test_args%"=="" goto test-v8
if "%config%"=="Debug" set test_args=--mode=debug %test_args%
if "%config%"=="Release" set test_args=--mode=release %test_args%
if defined no_cctest echo Skipping cctest because no-cctest was specified && goto run-test-py
if not exist %config%\cctest.exe goto run-test-py
if not exist "%config%\cctest.exe" echo cctest.exe not found. Run "vcbuild test" or "vcbuild cctest" to build it. && goto run-test-py
echo running 'cctest %cctest_args%'
"%config%\cctest" %cctest_args%
:run-test-py
Expand Down Expand Up @@ -635,7 +643,7 @@ del .used_configure_flags
goto exit

:help
echo vcbuild.bat [debug/release] [msi] [doc] [test/test-ci/test-all/test-addons/test-addons-napi/test-benchmark/test-internet/test-pummel/test-simple/test-message/test-tick-processor/test-known-issues/test-node-inspect/test-check-deopts/test-npm/test-async-hooks/test-v8/test-v8-intl/test-v8-benchmarks/test-v8-all] [ignore-flaky] [static/dll] [noprojgen] [projgen] [small-icu/full-icu/without-intl] [nobuild] [nosnapshot] [noetw] [ltcg] [nopch] [licensetf] [sign] [ia32/x86/x64] [vs2017] [download-all] [enable-vtune] [lint/lint-ci/lint-js/lint-js-ci/lint-md] [lint-md-build] [package] [build-release] [upload] [no-NODE-OPTIONS] [link-module path-to-module] [debug-http2] [debug-nghttp2] [clean] [no-cctest] [openssl-no-asm]
echo vcbuild.bat [debug/release] [msi] [doc] [test/test-ci/test-all/test-addons/test-addons-napi/test-benchmark/test-internet/test-pummel/test-simple/test-message/test-tick-processor/test-known-issues/test-node-inspect/test-check-deopts/test-npm/test-async-hooks/test-v8/test-v8-intl/test-v8-benchmarks/test-v8-all] [ignore-flaky] [static/dll] [noprojgen] [projgen] [small-icu/full-icu/without-intl] [nobuild] [nosnapshot] [noetw] [ltcg] [nopch] [licensetf] [sign] [ia32/x86/x64] [vs2017] [download-all] [enable-vtune] [lint/lint-ci/lint-js/lint-js-ci/lint-md] [lint-md-build] [package] [build-release] [upload] [no-NODE-OPTIONS] [link-module path-to-module] [debug-http2] [debug-nghttp2] [clean] [cctest] [no-cctest] [openssl-no-asm]
echo Examples:
echo vcbuild.bat : builds release build
echo vcbuild.bat debug : builds debug build
Expand Down