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

Fix incomplete/broken nghttp2 build support on Windows #1321

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@weltling
Contributor

weltling commented Mar 10, 2017

Things done

  • the flag name should be WITH_NGHTTP2, according to the current naming scheme. An external dep should always use "with" instead of "enable". The previous flag ENABLE_NGHTTP2 was kept for the compatibility reasons, so if passed it'll configure shared nghttp2 link as by default

  • the meaning of "with" was extended to allow shared/static link of nghttp2, the corresponding flags are passed as required by nghttp2.

  • nghttp2 does not depend on SSL of any kind, so the corresponding conditions are abandoned.

  • the build folder name is extended with "nghttp2-<dll|static>", as it's done for other libs

  • the library names are default as produced by the nghttp2 cmake configs.

thanks

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 10, 2017

@weltling, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @mback2k and @msnelling to be potential reviewers.

mention-bot commented Mar 10, 2017

@weltling, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @mback2k and @msnelling to be potential reviewers.

weltling added some commits Mar 10, 2017

@weltling weltling referenced this pull request Mar 10, 2017

Closed

Enable NGHTTP2 #5

@bagder bagder added the build label Mar 10, 2017

@rodwiddowson

This comment has been minimized.

Show comment
Hide comment
@rodwiddowson

rodwiddowson Mar 10, 2017

Contributor

I'll happily review these and try to give them a spin if @welting can eyeball #1259 - otherwise it will never progress.

Contributor

rodwiddowson commented Mar 10, 2017

I'll happily review these and try to give them a spin if @welting can eyeball #1259 - otherwise it will never progress.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Mar 10, 2017

Contributor

Thanks for keeping an eye, @rodwiddowson. This PR wasn't planned in the scope of #1322, but came out where i started the nghttp2 integration. I've just filed another one #1322, which targets the actual OpenSSL issue we have in a naive manner without breaking the current build approach. Basically, what were also discussed, is that one would keep the current approach, while making the lib names configurable. That's why I was wondering about #1259, as it seemed it were agreed to go by the suggestion to do it incrementally. We'll now need to choose a way to integrate these works somehow, but there will be for sure some merge conflicts. I would still suggest to go by an incremental integration, so we can thoroughly check every step and ensure the old way works, to hopefully get the whole accepted.

Thanks.

Contributor

weltling commented Mar 10, 2017

Thanks for keeping an eye, @rodwiddowson. This PR wasn't planned in the scope of #1322, but came out where i started the nghttp2 integration. I've just filed another one #1322, which targets the actual OpenSSL issue we have in a naive manner without breaking the current build approach. Basically, what were also discussed, is that one would keep the current approach, while making the lib names configurable. That's why I was wondering about #1259, as it seemed it were agreed to go by the suggestion to do it incrementally. We'll now need to choose a way to integrate these works somehow, but there will be for sure some merge conflicts. I would still suggest to go by an incremental integration, so we can thoroughly check every step and ensure the old way works, to hopefully get the whole accepted.

Thanks.

@bagder bagder closed this in df45f2c May 24, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 24, 2017

Member

Thanks, squashed and merged!

Member

bagder commented May 24, 2017

Thanks, squashed and merged!

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.