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

build: harmonize Clang checks and update V8 warning cflags #52873

Closed
wants to merge 2 commits into from
Closed

build: harmonize Clang checks and update V8 warning cflags #52873

wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented May 7, 2024

  • Set the clang variable in config.gypi so it depends on compiler
    checks made by the configure script.
  • Replace gyp conditions with llvm_version and "0.0" with conditions
    that use the clang variable.
  • Always use clang==1 or clang==0 in gyp conditions

This will help for #52870

@targos
build: harmonize Clang checks
791b8ff 
- Set the clang variable in `config.gypi` so it depends on compiler
  checks made by the configure script.
- Replace gyp conditions with `llvm_version` and "0.0" with conditions
  that use the `clang` variable.
- Always use `clang==1` or `clang==0` in gyp conditions
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. 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. zlib Issues and PRs related to the zlib subsystem. labels May 7, 2024
@targos targos added the wip Issues and PRs that are still a work in progress. label May 7, 2024
@targos
Copy link
Member Author

targos commented May 7, 2024

Somehow it broke test-asan.

@targos
Copy link
Member Author

targos commented May 7, 2024

Trying to remove as many -W flags as possible from the V8 build config. I'll re-add them if necessary.

@targos targos requested a review from richardlau May 7, 2024 14:05
@nodejs-github-bot
Copy link
Collaborator

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

@targos targos removed the wip Issues and PRs that are still a work in progress. label May 7, 2024
@targos targos changed the title build: harmonize Clang checks build: harmonize Clang checks and update V8 warning cflags May 7, 2024
@targos targos mentioned this pull request May 7, 2024
Merged
@targos targos 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. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels May 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 4338a28...2b657cc

nodejs-github-bot pushed a commit that referenced this pull request May 9, 2024
@targos @nodejs-github-bot
build: harmonize Clang checks
25c7880 
- Set the clang variable in `config.gypi` so it depends on compiler
  checks made by the configure script.
- Replace gyp conditions with `llvm_version` and "0.0" with conditions
  that use the `clang` variable.
- Always use `clang==1` or `clang==0` in gyp conditions

PR-URL: #52873
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request May 9, 2024
@targos @nodejs-github-bot
build: sync V8 warning cflags with BUILD.gn
2b657cc 
PR-URL: #52873
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@targos targos deleted the clang-config branch May 9, 2024 12:12
targos added a commit that referenced this pull request May 11, 2024
@targos
build: harmonize Clang checks
eed9674 
- Set the clang variable in `config.gypi` so it depends on compiler
  checks made by the configure script.
- Replace gyp conditions with `llvm_version` and "0.0" with conditions
  that use the `clang` variable.
- Always use `clang==1` or `clang==0` in gyp conditions

PR-URL: #52873
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
targos added a commit that referenced this pull request May 11, 2024
@targos
build: sync V8 warning cflags with BUILD.gn
daeb7db 
PR-URL: #52873
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@StefanStojanovic StefanStojanovic mentioned this pull request May 13, 2024
Open
12 tasks
@targos targos mentioned this pull request May 13, 2024
Merged
@zcbenz zcbenz mentioned this pull request May 15, 2024
Merged
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
@targos @lukins-cz
build: harmonize Clang checks
0ca94bc 
- Set the clang variable in `config.gypi` so it depends on compiler
  checks made by the configure script.
- Replace gyp conditions with `llvm_version` and "0.0" with conditions
  that use the `clang` variable.
- Always use `clang==1` or `clang==0` in gyp conditions

PR-URL: nodejs#52873
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
@targos @lukins-cz
build: sync V8 warning cflags with BUILD.gn
f142986 
PR-URL: nodejs#52873
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@richardlau richardlau richardlau approved these changes

@VoltrexKeyva VoltrexKeyva VoltrexKeyva 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. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. 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. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@targos @nodejs-github-bot @anonrig @richardlau @VoltrexKeyva