Skip to content

Conversation

@legendecas
Copy link
Member

@legendecaslegendecas commented Jun 27, 2024

Gyp generated build files should be able to be built in either Release/Debug mode.

  • make: single entry build file, two configurations by cli:
    make -C out BUILDTYPE=Release and make -C out BUILDTYPE=Debug.
  • msbuild: single entry build file, two configurations by cli:
    msbuild node.sln /p:Configuration=Release and
    msbuild node.sln /p:Configuration=Debug.
  • ninja: two directories in out/, build with
    ninja -C out/Release or ninja -C out/Debug.

Variables that changes with either Release or Debug configuration
should be defined in a configuration level, instead of the root level.
Example:

node/common.gypi

Lines 132 to 133 in 8e5d88b

'configurations':{
'Debug':{

This fixes generating invalid build files.

Additionally, v8_gypfiles/toolchain.gypi duplicates defines in
v8_gypfiles/features.gypi. Remove the duplications in
toolchains.gypi

Fixes: #53446

Gyp generated build files can be built in either Release/Debug mode. - make: single directory, two configurations by cli: `make -C out BUILDTYPE=Release` and `make -C out BUILDTYPE=Debug`. - msbuild: single directory, two configurations by cli: `msbuild node.sln /p:Configuration=Release` and `msbuild node.sln /p:Configuration=Debug`. - ninja: two directories in `out/`, build with `ninja -C out/Release` or `ninja -C out/Debug`. Variables that changes with either Release or Debug configuration should be defined in a configuration level, instead of the root level. This fixes generating invalid build files. Additionally, `v8_gypfiles/toolchain.gypi` duplicates defines in `v8_gypfiles/features.gypi`. Remove the duplications in `toolchains.gypi`
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jun 27, 2024
@legendecas
Copy link
MemberAuthor

./configure --debug option is not removed as it sets the default build type as well.

Copy link
Member

@targostargos left a comment

Choose a reason for hiding this comment

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

LGTM, but it doesn't change the fact that ./configure --ninja --debug && make still builds in Release and Debug modes, right?

Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
@legendecas
Copy link
MemberAuthor

it doesn't change the fact that ./configure --ninja --debug && make still builds in Release and Debug modes, right?

Yes, it still builds both Release and Debug products.

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2024
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 8, 2024

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

Rebasing on to latest main since there were reverts on reproducible snapshot.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 9, 2024

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

Restarting since #53755 landed addressing the snapshot reproducible test.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
MemberAuthor

Tried to reproduce locally and skip the sea test if the debug build is too large: #53918

@nodejs-github-bot
Copy link
Collaborator

@legendecaslegendecas added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 23, 2024
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/53605 ✔ Done loading data for nodejs/node/pull/53605 ----------------------------------- PR info ------------------------------------ Title build: fix gyp configs in debug (#53605) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch legendecas:build/debug-vars -> nodejs:main Labels build, v8 engine, tools, author ready, needs-ci Commits 2 - build: fix conflict gyp configs - fixup! build: fix gyp configs in debug Committers 2 - Chengzhong Wu <[email protected]> - GitHub <[email protected]> PR-URL: https://github.com/nodejs/node/pull/53605 Fixes: https://github.com/nodejs/node/issues/53446 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/53605 Fixes: https://github.com/nodejs/node/issues/53446 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 27 Jun 2024 14:24:05 GMT ✔ Approvals: 2 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/53605#pullrequestreview-2149988265 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53605#pullrequestreview-2151985821 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-07-22T21:41:04Z: https://ci.nodejs.org/job/node-test-pull-request/60543/ - Querying data for job/node-test-pull-request/60543/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 53605 From https://github.com/nodejs/node * branch refs/pull/53605/merge -> FETCH_HEAD ✔ Fetched commits as b19a9501020c..0e1eadd17396 -------------------------------------------------------------------------------- Auto-merging configure.py [main 63ebb41ad3] build: fix conflict gyp configs Author: Chengzhong Wu <[email protected]> Date: Wed Jun 26 15:20:24 2024 +0100 3 files changed, 15 insertions(+), 12 deletions(-) Auto-merging configure.py [main 9f4cabf169] fixup! build: fix gyp configs in debug Author: Chengzhong Wu <[email protected]> Date: Mon Jul 1 12:07:20 2024 +0100 1 file changed, 2 insertions(+), 2 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4) 

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
build: fix conflict gyp configs

Gyp generated build files can be built in either Release/Debug mode.

  • make: single directory, two configurations by cli:
    make -C out BUILDTYPE=Release and make -C out BUILDTYPE=Debug.
  • msbuild: single directory, two configurations by cli:
    msbuild node.sln /p:Configuration=Release and
    msbuild node.sln /p:Configuration=Debug.
  • ninja: two directories in out/, build with
    ninja -C out/Release or ninja -C out/Debug.

Variables that changes with either Release or Debug configuration
should be defined in a configuration level, instead of the root level.
This fixes generating invalid build files.

Additionally, v8_gypfiles/toolchain.gypi duplicates defines in
v8_gypfiles/features.gypi. Remove the duplications in
toolchains.gypi

PR-URL: #53605
Fixes: #53446
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>

[detached HEAD e2f983717b] build: fix conflict gyp configs
Author: Chengzhong Wu <[email protected]>
Date: Wed Jun 26 15:20:24 2024 +0100
3 files changed, 15 insertions(+), 12 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! build: fix gyp configs in debug

Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
PR-URL: #53605
Fixes: #53446
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>

[detached HEAD 17303f6e5a] fixup! build: fix gyp configs in debug
Author: Chengzhong Wu <[email protected]>
Date: Mon Jul 1 12:07:20 2024 +0100
1 file changed, 2 insertions(+), 2 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10054618263

@legendecaslegendecas added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 23, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 23, 2024
@nodejs-github-botnodejs-github-bot merged commit e192a32 into nodejs:mainJul 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in e192a32

@legendecaslegendecas deleted the build/debug-vars branch July 23, 2024 08:58
targos pushed a commit that referenced this pull request Jul 28, 2024
Gyp generated build files can be built in either Release/Debug mode. - make: single directory, two configurations by cli: `make -C out BUILDTYPE=Release` and `make -C out BUILDTYPE=Debug`. - msbuild: single directory, two configurations by cli: `msbuild node.sln /p:Configuration=Release` and `msbuild node.sln /p:Configuration=Debug`. - ninja: two directories in `out/`, build with `ninja -C out/Release` or `ninja -C out/Debug`. Variables that changes with either Release or Debug configuration should be defined in a configuration level, instead of the root level. This fixes generating invalid build files. Additionally, `v8_gypfiles/toolchain.gypi` duplicates defines in `v8_gypfiles/features.gypi`. Remove the duplications in `toolchains.gypi` PR-URL: #53605Fixes: #53446 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Jul 30, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
Gyp generated build files can be built in either Release/Debug mode. - make: single directory, two configurations by cli: `make -C out BUILDTYPE=Release` and `make -C out BUILDTYPE=Debug`. - msbuild: single directory, two configurations by cli: `msbuild node.sln /p:Configuration=Release` and `msbuild node.sln /p:Configuration=Debug`. - ninja: two directories in `out/`, build with `ninja -C out/Release` or `ninja -C out/Debug`. Variables that changes with either Release or Debug configuration should be defined in a configuration level, instead of the root level. This fixes generating invalid build files. Additionally, `v8_gypfiles/toolchain.gypi` duplicates defines in `v8_gypfiles/features.gypi`. Remove the duplications in `toolchains.gypi` PR-URL: #53605Fixes: #53446 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 21, 2024
Gyp generated build files can be built in either Release/Debug mode. - make: single directory, two configurations by cli: `make -C out BUILDTYPE=Release` and `make -C out BUILDTYPE=Debug`. - msbuild: single directory, two configurations by cli: `msbuild node.sln /p:Configuration=Release` and `msbuild node.sln /p:Configuration=Debug`. - ninja: two directories in `out/`, build with `ninja -C out/Release` or `ninja -C out/Debug`. Variables that changes with either Release or Debug configuration should be defined in a configuration level, instead of the root level. This fixes generating invalid build files. Additionally, `v8_gypfiles/toolchain.gypi` duplicates defines in `v8_gypfiles/features.gypi`. Remove the duplications in `toolchains.gypi` PR-URL: #53605Fixes: #53446 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Sep 30, 2024
@targostargos added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Oct 2, 2024
jazelly pushed a commit to jazelly/node that referenced this pull request Oct 2, 2024
Gyp generated build files can be built in either Release/Debug mode. - make: single directory, two configurations by cli: `make -C out BUILDTYPE=Release` and `make -C out BUILDTYPE=Debug`. - msbuild: single directory, two configurations by cli: `msbuild node.sln /p:Configuration=Release` and `msbuild node.sln /p:Configuration=Debug`. - ninja: two directories in `out/`, build with `ninja -C out/Release` or `ninja -C out/Debug`. Variables that changes with either Release or Debug configuration should be defined in a configuration level, instead of the root level. This fixes generating invalid build files. Additionally, `v8_gypfiles/toolchain.gypi` duplicates defines in `v8_gypfiles/features.gypi`. Remove the duplications in `toolchains.gypi` PR-URL: nodejs#53605Fixes: nodejs#53446 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Oct 16, 2024
codebytere added a commit to electron/electron that referenced this pull request Oct 21, 2024
codebytere added a commit to electron/electron that referenced this pull request Oct 21, 2024
codebytere added a commit to electron/electron that referenced this pull request Oct 22, 2024
codebytere added a commit to electron/electron that referenced this pull request Oct 28, 2024
codebytere added a commit to electron/electron that referenced this pull request Oct 29, 2024
codebytere added a commit to electron/electron that referenced this pull request Oct 31, 2024
jkleinsc pushed a commit to electron/electron that referenced this pull request Nov 4, 2024
* chore: bump Node.js to v22.9.0 * build: drop base64 dep in GN build nodejs/node#52856 * build,tools: make addons tests work with GN nodejs/node#50737 * fs: add fast api for InternalModuleStat nodejs/node#51344 * src: move package_json_reader cache to c++ nodejs/node#50322 * crypto: disable PKCS#1 padding for privateDecrypt nodejs-private/node-private#525 * src: move more crypto code to ncrypto nodejs/node#54320 * crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey nodejs/node#50234 * src: shift more crypto impl details to ncrypto nodejs/node#54028 * src: switch crypto APIs to use Maybe<void> nodejs/node#54775 * crypto: remove DEFAULT_ENCODING nodejs/node#47182 * deps: update libuv to 1.47.0 nodejs/node#50650 * build: fix conflict gyp configs nodejs/node#53605 * lib,src: drop --experimental-network-imports nodejs/node#53822 * esm: align sync and async load implementations nodejs/node#49152 * esm: remove unnecessary toNamespacedPath calls nodejs/node#53656 * module: detect ESM syntax by trying to recompile as SourceTextModule nodejs/node#52413 * test: adapt debugger tests to V8 11.4 nodejs/node#49639 * lib: update usage of always on Atomics API nodejs/node#49639 * test: adapt test-fs-write to V8 internal changes nodejs/node#49639 * test: adapt to new V8 trusted memory spaces nodejs/node#50115 * deps: update libuv to 1.47.0 nodejs/node#50650 * src: use non-deprecated v8::Uint8Array::kMaxLength nodejs/node#50115 * src: update default V8 platform to override functions with location nodejs/node#51362 * src: add missing TryCatch nodejs/node#51362 * lib,test: handle new Iterator global nodejs/node#51362 * src: use non-deprecated version of CreateSyntheticModule nodejs/node#50115 * src: remove calls to recently deprecated V8 APIs nodejs/node#52996 * src: use new V8 API to define stream accessor nodejs/node#53084 * src: do not use deprecated V8 API nodejs/node#53084 * src: do not use soon-to-be-deprecated V8 API nodejs/node#53174 * src: migrate to new V8 interceptors API nodejs/node#52745 * src: use supported API to get stalled TLA messages nodejs/node#51362 * module: print location of unsettled top-level await in entry points nodejs/node#51999 * test: make snapshot comparison more flexible nodejs/node#54375 * test: do not set concurrency on parallelized runs nodejs/node#52177 * src: move FromNamespacedPath to path.cc nodejs/node#53540 * test: adapt to new V8 trusted memory spaces nodejs/node#50115 * build: add option to enable clang-cl on Windows nodejs/node#52870 * chore: fixup patch indices * chore: add/remove changed files * esm: drop support for import assertions nodejs/node#54890 * build: compile with C++20 support nodejs/node#52838 * deps: update nghttp2 to 1.62.1 nodejs/node#52966 * src: parse inspector profiles with simdjson nodejs/node#51783 * build: add GN build files nodejs/node#47637 * deps,lib,src: add experimental web storage nodejs/node#52435 * build: add missing BoringSSL dep * src: rewrite task runner in c++ nodejs/node#52609 * fixup! build: add GN build files * src: stop using deprecated fields of v8::FastApiCallbackOptions nodejs/node#54077 * fix: shadow variable * build: add back incorrectly removed SetAccessor patch * fixup! fixup! build: add GN build files * crypto: fix integer comparison in crypto for BoringSSL * src,lib: reducing C++ calls of esm legacy main resolvenodejs/node#48325 * src: move more crypto_dh.cc code to ncrypto nodejs/node#54459 * chore: fixup GN files for previous commit * src: move more crypto code to ncrypto nodejs/node#54320 * Fixup Perfetto ifdef guards * fix: missing electron_natives dep * fix: node_use_node_platform = false * fix: include src/node_snapshot_stub.cc in libnode * 5507047: [import-attributes] Remove support for import assertions https://chromium-review.googlesource.com/c/v8/v8/+/5507047 * fix: restore v8-sandbox.h in filenames.json * fix: re-add original-fs generation logic * fix: ngtcp2 openssl dep * test: try removing NAPI_VERSION undef * chore(deps): bump @types/node * src: move more crypto_dh.cc code to ncrypto nodejs/node#54459 * esm: remove unnecessary toNamespacedPath calls nodejs/node#53656 * buffer: fix out of range for toString nodejs/node#54553 * lib: rewrite AsyncLocalStorage without async_hooks nodejs/node#48528 * module: print amount of load time of a cjs module nodejs/node#52213 * test: skip reproducible snapshot test on 32-bit nodejs/node#53592 * fixup! src: move more crypto_dh.cc code to ncrypto * test: adjust emittedUntil return type * chore: remove redundant wpt streams patch * fixup! chore(deps): bump @types/node * fix: gn executable name on Windows * fix: build on Windows * fix: rename conflicting win32 symbols in //third_party/sqlite On Windows otherwise we get: lld-link: error: duplicate symbol: sqlite3_win32_write_debug >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:47987 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_sleep >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48042 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_is_nt >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48113 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_unicode >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48470 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_unicode_to_utf8 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48486 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_mbcs_to_utf8 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48502 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_mbcs_to_utf8_v2 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48518 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_mbcs >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48534 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj lld-link: error: duplicate symbol: sqlite3_win32_utf8_to_mbcs_v2 >>> defined at .\..\..\third_party\electron_node\deps\sqlite\sqlite3.c:48550 >>> obj/third_party/electron_node/deps/sqlite/sqlite/sqlite3.obj >>> defined at obj/third_party/sqlite\chromium_sqlite3/sqlite3_shim.obj * docs: remove unnecessary ts-expect-error after types bump * src: move package resolver to c++ nodejs/node#50322 * build: set ASAN detect_container_overflow=0 nodejs/node#55584 * chore: fixup rebase * test: disable failing ASAN test * win: almost fix race detecting ESRCH in uv_kill libuv/libuv#4341
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.dont-land-on-v20.xPRs that should not land on the v20.x-staging branch and should not be released in v20.x.needs-ciPRs that need a full CI run.toolsIssues and PRs related to the tools directory.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node_mksnapshot debug build segfault

5 participants

@legendecas@nodejs-github-bot@jasnell@targos@VoltrexKeyva