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: add BUILD_TESTING, fix MSVC with static + shared #213

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

donny-dont
Copy link
Contributor

@donny-dont donny-dont commented Mar 26, 2024

Add a new option BUILD_TESTING to control whether or not the library is built with testing. Also fix a library naming conflict when building both static and shared libraries in MSVC by setting a default of _static for STATIC_LIB_SUFFIX.

@donny-dont
Copy link
Contributor Author

Same as nghttp2/nghttp2#2092 but just for nghttp3. And same as ngtcp2/ngtcp2#1189

@tatsuhiro-t
Copy link
Member

cc @vszakats

@vszakats
Copy link
Contributor

I'd suggest changing the description to cmake: add BUILD_TESTING, fix MSVC with static + shared or similar.
ENABLE_STATIC_LIB was already respected before this patch, existing title suggests it wasn't.

[ Since https://github.com/nghttp2/nghttp2/pull/2092 renamed the shared/static enabler variables (I'm not sure about the reasons), now we have a disparity between nghttp2 and nghttp3/ngtcp2. If we want to keep parity, we'd either need to rename those variables in the latter two (breaking compatibility for everyone), or rename back in nghttp2. ]

@donny-dont donny-dont changed the title CMake: Respect ENABLE_STATIC_LIB cmake: add BUILD_TESTING, fix MSVC with static + shared Mar 27, 2024
@donny-dont
Copy link
Contributor Author

I'd suggest changing the description to cmake: add BUILD_TESTING, fix MSVC with static + shared or similar. ENABLE_STATIC_LIB was already respected before this patch, existing title suggests it wasn't.

Fixed and updated the PR title and description.

[ Since https://github.com/nghttp2/nghttp2/pull/2092 renamed the shared/static enabler variables (I'm not sure about the reasons), now we have a disparity between nghttp2 and nghttp3/ngtcp2. If we want to keep parity, we'd either need to rename those variables in the latter two (breaking compatibility for everyone), or rename back in nghttp2. ]

The actual change happened in nghttp2/nghttp2@d9edee4 which I'm guessing was to align with CMake's built-in variable BUILD_SHARED_LIBS. I didn't want to make that change on anyone's behalf. However the new variable BUILD_TESTING does conform with CMake's conventions.

Add a new option `BUILD_TESTING` to control whether or not the library is built with testing. Also fix a library naming conflict when building both static and shared libraries in MSVC by setting a default of `_static` for `STATIC_LIB_SUFFIX`.
@vszakats
Copy link
Contributor

vszakats commented Mar 27, 2024

I'd suggest changing the description to cmake: add BUILD_TESTING, fix MSVC with static + shared or similar. ENABLE_STATIC_LIB was already respected before this patch, existing title suggests it wasn't.

Fixed and updated the PR title and description.

Thank you, LGTM!

[ Since https://github.com/nghttp2/nghttp2/pull/2092 renamed the shared/static enabler variables (I'm not sure about the reasons), now we have a disparity between nghttp2 and nghttp3/ngtcp2. If we want to keep parity, we'd either need to rename those variables in the latter two (breaking compatibility for everyone), or rename back in nghttp2. ]

The actual change happened in nghttp2/nghttp2@d9edee4 which I'm guessing was to align with CMake's built-in variable BUILD_SHARED_LIBS. I didn't want to make that change on anyone's behalf. However the new variable BUILD_TESTING does conform with CMake's conventions.

Agreed with BUILD_TESTING. And leaving the rest as-is. (Whatever the resolution is for the renames, it's something for a different patch.)

vszakats added a commit to curl/curl-for-win that referenced this pull request Mar 29, 2024
For good measure (they were implicitly not built before this patch).

Support pending for nghttp3 and ngtcp2:
ngtcp2/nghttp3#213
ngtcp2/ngtcp2#1189
@tatsuhiro-t tatsuhiro-t added this to the v1.3.0 milestone Mar 29, 2024
@tatsuhiro-t tatsuhiro-t merged commit f7fc0e1 into ngtcp2:main Mar 29, 2024
14 checks passed
@tatsuhiro-t
Copy link
Member

Thank you both. Merged now.

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 this pull request may close these issues.

None yet

3 participants