Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

build: use nodejs/http_parser as a external dep #591

Merged
merged 5 commits into from
Aug 21, 2020

Conversation

neverchanje
Copy link
Contributor

No description provided.

URL_MD5 f6900b9209d3d6b80c70e050ac33b834
CONFIGURE_COMMAND mkdir -p ${TP_OUTPUT}/include/nodejs
BUILD_COMMAND ""
INSTALL_COMMAND cp -R http_parser.h ${TP_OUTPUT}/include/nodejs/http_parser.h
Copy link
Member

Choose a reason for hiding this comment

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

This thirdparty is only copied to output directory but compiled in rdsn project?

Copy link
Contributor Author

@neverchanje neverchanje Aug 17, 2020

Choose a reason for hiding this comment

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

Yes. We don't compile http_parser as "libhttp_parser.a", otherwise we must link this library to every place. This PR only modifies where the source is placed, from our git to 3rdparties.

Copy link
Member

Choose a reason for hiding this comment

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

Only http related modules need link this library, I think keep logic clear and unified would be better.

Copy link
Contributor Author

@neverchanje neverchanje Aug 20, 2020

Choose a reason for hiding this comment

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

"Only http related modules need link this library"

Yes, which means meta-server, replica-server, and all related modules need to link this library. It's not trivial.

If we don't make it an external dependency, I will paste the http-parser v2.9.4 sources to replace the current.

@neverchanje neverchanje merged commit 96f32ce into XiaoMi:master Aug 21, 2020
@neverchanje neverchanje deleted the http-refactoring-parser-cmake branch August 21, 2020 06:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants