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

NGHTTP2: Add -DNGHTTP2_STATICLIB if static #4670

Closed
wants to merge 1 commit into from
Closed

Conversation

1480c1
Copy link
Contributor

@1480c1 1480c1 commented Dec 3, 2019

Add NGHTTP2_STATICLIB to compile flags and to the pkg-config
if static only since that flag is required to make sure curl
pulls the static versions of the symbols. Helps with dependencies
to not need to add NGHTTP2_STATICLIB everytime curl is linked.

I don't know if the automake sections are correct since I haven't messed with it before.

Add NGHTTP2_STATICLIB to compile flags and to the pkg-config
if static only since that flag is required to make sure curl
pulls the static versions of the symbols. Helps with dependencies
to not need to add NGHTTP2_STATICLIB everytime curl is linked.
@@ -394,6 +394,10 @@ if(USE_NGHTTP2)
find_package(NGHTTP2 REQUIRED)
include_directories(${NGHTTP2_INCLUDE_DIRS})
list(APPEND CURL_LIBS ${NGHTTP2_LIBRARIES})
if(NOT BUILD_SHARED_LIBS)
Copy link
Member

Choose a reason for hiding this comment

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

Does't BUILD_SHARED_LIBS imply that you build libcurl statically and it doesn't actually say anything about the dependencies?

Copy link
Contributor Author

@1480c1 1480c1 Dec 3, 2019

Choose a reason for hiding this comment

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

Indeed, BUILD_SHARED_LIBS only affects add_library and doesn't affect find_library and other, but I could not think of a better variable to tie it to other than creating a new option, something like STATIC_NGHTTP2 or similar to force static compilation by finding the static libs and cflags, but I am not sure if you guys are fine with adding new cmake options.

I would much prefer to add something like a STATIC_${DEPS} options, but would like to hear feedback about something like that before implementing, especially since it not something that is represented in the configure file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like 1480c1@37ada3e instead. Although, I am not sure how to do this in automake yet.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I figure it could be done with a --enable-static-nghttp2 option in a very similar way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then should I do that instead?

Copy link
Member

@bagder bagder Dec 4, 2019

Choose a reason for hiding this comment

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

Well, I'm not convinced it is a terribly motivated change. You could also just invoke CPPFLAGS=-DNGHTTP2_STATICLIB ./configure and then you wouldn't have to change configure at all...

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever you do, please don't tie it to BUILD_SHARED_LIBS. The fact that you want to build static curl does not imply that you want all the dependencies to be static.

@@ -3315,6 +3317,8 @@ case "$OPT_H2" in
esac

curl_h2_msg="disabled (--with-nghttp2)"
AM_CONDITIONAL([USE_CPPFLAG_NGHTTP2_STATICLIB],
[test "x$xc_lt_build_static_only" = 'xyes'])
Copy link
Member

Choose a reason for hiding this comment

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

Here too, this seems to use the request to build a static-only version of curl to suggest that nghttp2 is also static, which it doesn't say!

@1480c1
Copy link
Contributor Author

1480c1 commented Dec 5, 2019

After running all of my compilation without writing -DNGHTTP2_STATICLIB to the pkg-config file, none of the dependencies errored out like before, so it doesn't seem this is necessary anymore

@1480c1 1480c1 closed this Dec 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants