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

cmake fails to build with -DZSTD_PROGRAMS_LINK_SHARED=ON on windows #3554

Closed
anarazel opened this issue Mar 15, 2023 · 8 comments · Fixed by #3739
Closed

cmake fails to build with -DZSTD_PROGRAMS_LINK_SHARED=ON on windows #3554

anarazel opened this issue Mar 15, 2023 · 8 comments · Fixed by #3739

Comments

@anarazel
Copy link

Hi,

building with -DZSTD_PROGRAMS_LINK_SHARED=ON results in:

fileio_asyncio.c.obj : error LNK2019: unresolved external symbol POOL_create referenced in function AIO_IOPool_createThreadPool
fileio_asyncio.c.obj : error LNK2019: unresolved external symbol POOL_free referenced in function AIO_IOPool_destroy
fileio_asyncio.c.obj : error LNK2019: unresolved external symbol POOL_joinJobs referenced in function AIO_IOPool_join
fileio_asyncio.c.obj : error LNK2019: unresolved external symbol POOL_add referenced in function AIO_IOPool_enqueueJob
programs\zstd.exe : fatal error LNK1120: 4 unresolved externals

The reason is fairly simple: The API in pool.h isn't made visible, therefore the references from programs/fileio.c can't see them.

Before 1598e6c a build like this worked. CC: @yoniko

It looks like it'd be easy enough to fix for POOL_create, POOL_free, there are public wrappers. But for the rest there isn't an obvious answer.

Regards,

Andres

@yoniko
Copy link
Contributor

yoniko commented Mar 15, 2023

Thank you for reporting @anarazel .
I haven't been able to reproduce on my machine yet, but perhaps there are some differences that would explain that.

  1. Looks like you are running, does this happen on other platforms? sadly I don't have a windows machine to test.
  2. A similar issue has been fixed in Fix zstd-dll build missing dependencies #3496, are you trying to compile dev or the 1.5.4 release?

@anarazel
Copy link
Author

@yoniko Thanks for the quick reply.

Looks like you are running, does this happen on other platforms? sadly I don't have a windows machine to test.

I haven't seen it on other platforms, but I haven't tried hard. I normally am not a windows person, I am just working on CI stuff, forcing me to touch windows things (I want to use the zstd binary in tests, hence modifying the vcpkg port to allow that, hitting this issue).

A similar issue has been fixed in #3496, are you trying to compile dev or the 1.5.4 release?

It reproduces on both dev and 1.5.4. It does not reproduce on v1.5.2.

I think a similar approach to #3496 could be implemented for the cmake build.

@anarazel
Copy link
Author

I think a similar approach to #3496 could be implemented for the cmake build.

That indeed works. Oddly enough, zstd-dll also has a custom xxhash.c reference, which doesn't seem to be required with msvc.

Ah, and I see why this doesn't fail on !windows: While lib/Makefile adds -fvisibility=hidden, build/cmake/lib/CMakeLists.txt doesn't.

If you add set_target_properties(libzstd_shared PROPERTIES C_VISIBILITY_PRESET hidden) it fails on linux as well:

/usr/bin/ld: programs/CMakeFiles/zstd.dir/home/andres/src/zstd/programs/fileio_asyncio.c.o: in function `AIO_IOPool_init':
fileio_asyncio.c:(.text+0xde): undefined reference to `POOL_create'
/usr/bin/ld: programs/CMakeFiles/zstd.dir/home/andres/src/zstd/programs/fileio_asyncio.c.o: in function `AIO_WritePool_enqueueAndReacquireWriteJob':
fileio_asyncio.c:(.text+0xf67): undefined reference to `POOL_add'
...

Which is nice, because it allows fixing this outside of windows :)

@anarazel
Copy link
Author

I'd open a PR with the, pretty obvious, fix - but it looks like I'd need to get legal involved due to the CLA.

@yoniko
Copy link
Contributor

yoniko commented Mar 16, 2023

Sounds good, a PR would be very much appreciated.
I'd also suggest adding a CI test for this scenario, extending the cmake-visual-2022 job to include a run with this flag would probably be the easiest.

@anarazel
Copy link
Author

I'll check what legal says about the CLA.

@anarazel
Copy link
Author

I'll check what legal says about the CLA.

(still waiting, sorry for that)

@jaimergp
Copy link

Any news @anarazel? We are also hitting this in conda-forge and would love a patch 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants