Skip to content

build: only generate specified build type ninja files#53511

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
legendecas:build/ninja
Jun 22, 2024
Merged

build: only generate specified build type ninja files#53511
nodejs-github-bot merged 1 commit intonodejs:mainfrom
legendecas:build/ninja

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented Jun 19, 2024 •
edited
Loading

Release and Debug build configurations can not be shared, only
generate specified configuration in configure with the ninja
generator. This avoids generating invalid DEBUG ninja build files
when configure was not run with --debug.

Fixes: #53446

@legendecas
build: only generate specified build type files
95a97e1 
Release and Debug build configurations can not be shared, only
generate specified configuration in `configure`.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Jun 19, 2024
@legendecas
Copy link
Member Author

legendecas commented Jun 19, 2024 •
edited
Loading

Specifically, setting v8_enable_v8_checks in generated config.gypi to 1 and regenerate ninja files could fix the issue. Anyway, https://github.com/nodejs/node/blob/main/doc/contributing/building-node-with-ninja.md#producing-a-debug-build suggested that build with ./configure --ninja --debug && make.

@legendecas legendecas requested a review from targos June 19, 2024 15:24
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 19, 2024

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

joyeecheung
joyeecheung reviewed Jun 20, 2024
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 20, 2024

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

@legendecas legendecas added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 21, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2024
@nodejs-github-bot nodejs-github-bot merged commit 6cb940a into nodejs:main Jun 22, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2024

Landed in 6cb940a

@legendecas legendecas deleted the build/ninja branch June 22, 2024 08:41
@targos
Copy link
Member

targos commented Jun 25, 2024

I should have tried it before approving. I'm not able to work with ninja and make after this change:

$ python3 configure.py --ninja --debug --node-builtin-modules-path=`pwd`
Warning! Loading builtin modules from disk is for development
INFO: configure completed successfully

$ make -j6
gmake: *** No rule to make target 'out/Release/build.ninja', needed by 'node'.  Stop.

$ make -j6 BUILDTYPE=Debug
gmake: *** No rule to make target 'out/Release/build.ninja', needed by 'node'.  Stop.

targos pushed a commit that referenced this pull request Jun 25, 2024
@legendecas @targos
build: only generate specified build type files
1a3ccba 
Release and Debug build configurations can not be shared, only
generate specified configuration in `configure`.

PR-URL: #53511
Fixes: #53446
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@legendecas legendecas mentioned this pull request Jun 25, 2024
Merged
@legendecas
Copy link
Member Author

legendecas commented Jun 25, 2024

Yeah, make BUILDTYPE=Debug still builds in release mode so I think an alternative should be making the generated config.gypi works in both Release mode and Debug mode. Reverting since this is breaking existing work flow.

@targos targos added the dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. label Jun 25, 2024
@aduh95 aduh95 mentioned this pull request Oct 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@anonrig anonrig anonrig approved these changes

@targos targos targos approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

node_mksnapshot debug build segfault

7 participants

@legendecas @nodejs-github-bot @targos @jasnell @lpinca @anonrig @joyeecheung