node icon indicating copy to clipboard operation
node copied to clipboard

build: fix gyp configs in debug

Open legendecas opened this issue 1 year ago • 2 comments

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: https://github.com/nodejs/node/blob/8e5d88b6413558b6b4bc127f87fc8122311639ae/common.gypi#L132-L133

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: https://github.com/nodejs/node/issues/53446

legendecas avatar Jun 27 '24 14:06 legendecas

Review requested:

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

nodejs-github-bot avatar Jun 27 '24 14:06 nodejs-github-bot

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

legendecas avatar Jun 27 '24 14:06 legendecas

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.

legendecas avatar Jul 01 '24 11:07 legendecas

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6058/

nodejs-github-bot avatar Jul 03 '24 23:07 nodejs-github-bot

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6058/

nodejs-github-bot avatar Jul 04 '24 00:07 nodejs-github-bot

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6058/

nodejs-github-bot avatar Jul 04 '24 00:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 04 '24 01:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 08 '24 20:07 nodejs-github-bot

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 avatar Jul 08 '24 23:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 09 '24 10:07 nodejs-github-bot

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

Restarting since https://github.com/nodejs/node/pull/53755 landed addressing the snapshot reproducible test.

nodejs-github-bot avatar Jul 09 '24 16:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 09 '24 18:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 10 '24 07:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 11 '24 18:07 nodejs-github-bot

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

nodejs-github-bot avatar Jul 18 '24 09:07 nodejs-github-bot

Tried to reproduce locally and skip the sea test if the debug build is too large: https://github.com/nodejs/node/pull/53918

legendecas avatar Jul 18 '24 11:07 legendecas

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

nodejs-github-bot avatar Jul 22 '24 21:07 nodejs-github-bot

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: 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]>

[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: 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]>

[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

nodejs-github-bot avatar Jul 23 '24 07:07 nodejs-github-bot

Landed in e192a32c27f223e7a3dcb43134fd891dd8ded760

nodejs-github-bot avatar Jul 23 '24 08:07 nodejs-github-bot