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

Static library always built with identical lib name as shared causing linking errors #2088

Closed
jonaski opened this issue Mar 2, 2024 · 6 comments · Fixed by #2092
Closed

Comments

@jonaski
Copy link
Contributor

jonaski commented Mar 2, 2024

As you have removed the if(HAVE_CUNIT OR ENABLE_STATIC_LIB) condition in version 1.60.0, it's always building the static library, even though I explicitly have been disabling it with ENABLE_STATIC_LIB=OFF, and since 1.60.0 with BUILD_STATIC_LIBS=OFF. STATIC_LIB_SUFFIX is never set, but is used here:

ARCHIVE_OUTPUT_NAME nghttp2${STATIC_LIB_SUFFIX}

But without STATIC_LIB_SUFFIX being set, the static library is using identical lib name as the shared lib causing linking errors, as you can see here: https://github.com/strawberrymusicplayer/strawberry-msvc-dependencies/actions/runs/8115697967/job/22184102638?pr=370
This means the user has to configure CMake with STATIC_LIB_SUFFIX to avoid the conflict when building a shared library.
The if(BUILD_STATIC_LIBS) line should be moved so it does not only apply to the install, but building too.
And STATIC_LIB_SUFFIX should probably be CMake option defaulting to "static" so it does create conflicts when building both shared and static.

@tatsuhiro-t
Copy link
Member

if(HAVE_CUNIT OR ENABLE_STATIC_LIB) was removed because CUnit dependency is dropped and the unit tests are always built which requires static lib if I understand it correctly.
Would you mind submitting a patch to fix the issue you described?

@jmckenna
Copy link

jmckenna commented Mar 2, 2024

Initially reported (and a fix) through #2051 (comment)

@jmckenna
Copy link

jmckenna commented Mar 2, 2024

We should probably notify all packagers of 1.60.0 that the if statement needs to be moved (specific line at #2051 (comment) )

@jmckenna
Copy link

jmckenna commented Mar 2, 2024

@jonaski do you want to submit a pull request? It seems your solution is more detailed than mine :) Mine is a quick fix for 1.60.0 packagers, but it sounds like you have the proper way. thanks.

@jmckenna
Copy link

jmckenna commented Mar 2, 2024

Also, I think we should be testing for this (I'm not familiar with the CI for windows for this project), on each commit, to prevent such a breaking change occurring in a release again.

@jonaski
Copy link
Contributor Author

jonaski commented Mar 2, 2024

I can do it.

jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 2, 2024
Respect BUILD_STATIC_LIBS, default to ON since the tests require it, but still it possible to turn it manually off, if BUILD_STATIC_LIBS is OFF do not include tests.

This also fixes the library conflict by removing the ARCHIVE_OUTPUT_NAME property so it's using "nghttp2_static", since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 2, 2024
Respect BUILD_STATIC_LIBS, default to ON since the tests require it, but still make it possible to turn it off, if BUILD_STATIC_LIBS is OFF do not include tests.

This also fixes the library conflict by removing the ARCHIVE_OUTPUT_NAME property so it's using "nghttp2_static", since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 3, 2024
Respect BUILD_STATIC_LIBS, default to ON since the tests require it, but still make it possible to turn it off, if BUILD_STATIC_LIBS is OFF do not include tests.

This also fixes the library conflict by removing the ARCHIVE_OUTPUT_NAME property so it's using "nghttp2_static", since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 3, 2024
Respect BUILD_STATIC_LIBS and add option for tests.

This also fixes the library conflict by removing the ARCHIVE_OUTPUT_NAME property so it's using "nghttp2_static", since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 3, 2024
Respect BUILD_STATIC_LIBS and add option for tests.

This also fixes the library conflict with MSVC by setting the STATIC_LIB_SUFFIX so it's using "nghttp2_static" with both static and shared library is built, since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 3, 2024
Respect BUILD_STATIC_LIBS and add option for tests.

This also fixes the library conflict with MSVC by setting STATIC_LIB_SUFFIX so it's using "nghttp2_static" when both static and shared library is built, since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
tatsuhiro-t pushed a commit that referenced this issue Mar 4, 2024
Respect BUILD_STATIC_LIBS and add option for tests.

This also fixes the library conflict with MSVC by setting STATIC_LIB_SUFFIX so it's using "nghttp2_static" when both static and shared library is built, since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes #2088
jonaski added a commit to jonaski/nghttp2 that referenced this issue Mar 4, 2024
Respect BUILD_STATIC_LIBS and add option for tests.

This also fixes the library conflict with MSVC by setting STATIC_LIB_SUFFIX so it's using "nghttp2_static" when both static and shared library is built, since the lib name currently conflicts with the shared when STATIC_LIB_SUFFIX was not set.

Fixes nghttp2#2088
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