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
Use public scope to fix shared windows build#4009
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Hi @uilianries! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Done, CLA has been signed. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@uilianries Zstd should not require the public include directory to build, because it uses relative includes. Is this a deficiency in the Windows cmake build, or have I misunderstood something about cmake? |
I'm happy to accept the PR, I would just like to understand why this is happening first |
@terrelln Sorry my delay. When building shared on Windows, in order to generate the DLL, the file libzstd-dll.rc will be consumed here: zstd/build/cmake/lib/CMakeLists.txt Line 125 in 72c16b1
Then, it requires zstd.h (the public header) too:
However, properties marked as interface are only visible to the consumers of the target, not to the target itself. The result is follow error, because that folder is not visible when building the shared library on Windows. Another option would be adding an expression to use PUBLIC or INTERFACE according to MSVC value, but I guess is too much. |
Thanks! I didn't even realize that that file existed! We've switched all of our other headers over to relative includes. Would it be possible to use a relative include for that file as well? But this makes sense, and I will merge this PR. Could you please just add a comment explaining why it needs to be |
@terrelln I just updated the PR description with more details. Please, take a look. About the relative include, do you mean changing the
It would work yes, it tested locally on Windows + CMake and compiled the DLL without errors. But is really fragile, as any layout change in the folder would break it. Plus, I see not only CMake is consuming the .rc file, but also Meson and VS2010, so would need to build with them too to make sure. |
There is also #4019. |
zstd 1.5.6 has issues building on Windows without the changes from github.com/facebook/zstd/pull/4009
I did run into the same issue. The PR worked for me. Thanks! |
#4019 was merged, so this PR might be obsolete. |
Hello all!
The CMake target
libzstd_shared
requires the filelibzstd-dll.rc
to expose library's symbols in DLL, when building with MSVC and configured to shared.zstd/build/cmake/lib/CMakeLists.txt
Line 125 in 72c16b1
Still, the file
libzstd-dll.rc
includes the public headerzstd.h
to be able to read those publuc methods.zstd/build/VS2010/libzstd-dll/libzstd-dll.rc
Line 4 in 72c16b1
The same does not occur with Linux/OSX, because only Windows needs that file to expose symbols.
To be able to access the
zstd.h
, CMake needs to know where this file is available. One way to do that is using thetarget_include_directories
method, pointing a folder to be included to that CMake target only. The same is configured with the scopeINTERFACE
, which is only visible to the consumers of the target, not to the target itself. The result is follow error, because that folder is not visible when building on Windows (the OS does not matter).In order to fix the situation, the
zstd.h
should be visible when both building the target, but also when consuming it. It describes thePUBLIC
scope.fixes #3999
OS: Windows 10
Compiler: Visual Studio 17 2022
CMake: version 3.29.0
This patch uses
PUBLIC
as scope to fix the current build.Here is the build log using https://github.com/facebook/zstd/releases/download/v1.5.6/zstd-1.5.6.tar.gz without the patch.
Details