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.

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

build: add vtune profiling enabling flag in default Linux buiding CI #46116

Closed
wants to merge 1 commit into from

Conversation

lucshi
Copy link

@lucshi lucshi commented Jan 6, 2023

Added --enable-vtune-profiling into test-linux.yml to avoid building broken when users enable vtune profiling. This check is done in CI and for Linux only so as to make the CI change minimal but effective.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jan 6, 2023
@bnoordhuis
Copy link
Member

I'm -1. This is only going to frustrate V8 upgrades more and those are frustrating enough already.

VTune support in node depends on V8's vtune support being in good shape but they don't test that regularly.

@lucshi
Copy link
Author

lucshi commented Jan 6, 2023

I'm -1. This is only going to frustrate V8 upgrades more and those are frustrating enough already.

VTune support in node depends on V8's vtune support being in good shape but they don't test that regularly.

This PR is echo for #45248 (comment) .Could you advice if CI is needed or you think it will be good enough that I fix bugs asap when there would be bugs in future?

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 6, 2023

You're of course welcome to be proactive in sending fixes, those are welcome. CI isn't a good solution because of the aforementioned upstream problem.

@lucshi
Copy link
Author

lucshi commented Jan 6, 2023

You're of course welcome to be proactive in sending fixes, those are welcome. CI isn't a good solution because of the aforementioned upstream problem.

I'm OK to close this PR if no special actions are needed to track whether VTune building is broken.

@bnoordhuis
Copy link
Member

I'll go ahead and close this then but thanks anyway!

@bnoordhuis bnoordhuis closed this Jan 6, 2023
@lucshi lucshi deleted the vtune-ci branch December 4, 2023 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants