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

doc: add tips about vcpkg cause build faild on windows #52181

Conversation

congzhangzh
Copy link
Contributor

vcpkg may cause nodejs build on windows to fail, symbol already defined in zlib.lib(zlib1.dll).

vcpkg uses some magic to hook into the vc++ project, add its include to include search dir and auto copy the dll to output dir if vcpkg integrate install is done.

some talk happens on the internet and none of them point to vcpkg, but one thing is sure, once you run vcpkg integrate remove, vcbuild.bat will succ!

vcpkg is popular and included in CLion and Visual Studio, so our folks should know this may be a trouble!

Refs:

  1. Multiply defined symbol errors in libuv.lib and openssl.lib because of node.lib help#1656
  2. Problem Building Node (had errors about multiply-defined symbols and such) #24448

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Mar 21, 2024
@congzhangzh
Copy link
Contributor Author

@H4ad Hi Vinicius, can you review this document?

@congzhangzh congzhangzh closed this by deleting the head repository Mar 22, 2024
@congzhangzh congzhangzh reopened this Mar 22, 2024
@H4ad
Copy link
Member

H4ad commented Mar 23, 2024

Since I never had this issue and I don't know how to reproduce it, I'm afraid that I should not approve this.

But let's ping the other members of @nodejs/platform-windows and see if someone feels more comfortable approving this one.

@StefanStojanovic
Copy link
Contributor

@congzhangzh I'd like to check this, but I too never had this issue. Do you have any idea how to reproduce the issue this tip solves?

@congzhangzh
Copy link
Contributor Author

congzhangzh commented Mar 26, 2024

@congzhangzh I'd like to check this, but I too never had this issue. Do you have any idea how to reproduce the issue this tip solves?

@StefanStojanovic Hi Stefan, that's great you can help:)

First, let me clarify how I identified this problem.

I have two windows to compile node, one work, and the other failed.
The failed one told me that symbol was redefined.
I use some trick to allow symbol redefine of link.exe, and build success with a warning! https://github.com/congzhangzh/daily-scripts-4-us/blob/38ff4ecefe19ef796a198a817aa7b773c715ae5e/ccache/Directory.Build.targets#L13
I use depends to check node depends, then a zlib.dll point to vcpkg installed package zlib, then I got it.

How to reproduce this problem

  1. install vcpkg
git clone https://github.com/microsoft/vcpkg
cd vcpkg
bootstrap-vcpkg.bat
  1. install zlib
: I use vcpkg install boost opentelem-cpp[oltp-http,oltp-grpc]
vcpkg install zlib
  1. enable vcpkg integrate
vcpkg integrate install
  1. build nodejs with vcbuild.bat
git clone https://github.com/nodejs/node.git
cd node
: I use vcbuild.bat x64 dll debug
vcbuild.bat x64 dll
  1. you should failed with symbol redefine
  2. disable vcpkg integrate
cd vcpkg_repo_dir
vcpkg integrate remove
  1. rebuild nodejs, you should success
cd node_repo_dir
: I use vcbuild.bat x64 dll debug
vcbuild.bat x64 dll
  1. if not, try to remove out dir, and rebuild
cd node_repo_dir
rmdir out /q /s /f
: I use vcbuild.bat x64 dll debug
vcbuild.bat x64 dll
  1. then at least vcpkg has some relation with nodejs build failed

Btw

that should be a misconfig of vcpkg .props/.target hook? can you cc to upstream?

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

I've created a fresh VM and tried out the scenario you suggested. All played out as you said it would, so although I've never experienced this before I can confirm your tip for fixing it works.

The remove command you have in the BUILDING.md isn't correct (the one in your comment is), so please change that and I'll approve the PR.

BUILDING.md Outdated Show resolved Hide resolved
Co-authored-by: Stefan Stojanovic <[email protected]>
@congzhangzh
Copy link
Contributor Author

I've created a fresh VM and tried out the scenario you suggested. All played out as you said it would, so although I've never experienced this before I can confirm your tip for fixing it works.

The remove command you have in the BUILDING.md isn't correct (the one in your comment is), so please change that and I'll approve the PR.

@StefanStojanovic Hi Stefan, that's cool, and I change that follow you advice, please review again :)

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

The change LGTM.

Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

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

Giving the approval on behalf of @StefanStojanovic, thanks for the great review, and @congzhangzh thanks for the PR.

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 3, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit dd711d2 into nodejs:main Apr 3, 2024
18 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in dd711d2

marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52181
Refs: nodejs/help#1656
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52181
Refs: nodejs/help#1656
Reviewed-By: Vinícius Lourenço Claro Cardoso <[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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants