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,tools,node-api: fix building node-api tests for Windows Debug #52632

Merged

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Apr 22, 2024

Currently when we run .\vcbuild Debug build-node-api-tests we are getting an error that says:
"Can not find out dir, did you run configure?"

The issue is that the tools/build_addons.py hardcodes Release output path for Windows and compiling for Debug fails because the out\Release folder does not exists. For debug config we must use out\Debug path instead.
This new code was introduced in PR #50737.

This PR fixes the issue by adding the new config parameter to the tools/build_addons.py which is used to find the default output directory. The vcbuild.bat is changed to pass the %config% variable to the tools/build_addons.py.

@vmoroz vmoroz requested a review from zcbenz April 22, 2024 02:16
@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. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Apr 22, 2024
@vmoroz vmoroz added the node-api Issues and PRs related to the Node-API. label Apr 22, 2024
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Contributor

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@vmoroz vmoroz force-pushed the pr/fix_node_api_tests_for_debug_config branch from 7cb971f to 2d78d59 Compare April 22, 2024 23:39
@vmoroz vmoroz added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2024
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@Flarna Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 26, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 26, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2334d5c into nodejs:main Apr 26, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2334d5c

@vmoroz vmoroz deleted the pr/fix_node_api_tests_for_debug_config branch April 27, 2024 14:22
aduh95 pushed a commit that referenced this pull request Apr 29, 2024
PR-URL: #52632
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Cheng Zhao <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52632
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Cheng Zhao <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52632
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Cheng Zhao <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
PR-URL: nodejs#52632
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Cheng Zhao <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants