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.

Sign up for GitHub

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

Jump to bottom

[brpc] Add new port #11524

Merged
merged 8 commits into from
Jul 31, 2020
Merged

[brpc] Add new port #11524

merged 8 commits into from
Jul 31, 2020

Conversation

curoky
Copy link
Contributor

@curoky curoky commented May 22, 2020

Industrial-grade RPC framework used throughout Baidu, with 1,000,000+ instances and thousands kinds of services, called "baidu-rpc" inside Baidu.

https://github.com/apache/incubator-brpc

What does your PR fix? Fixes issue #
N/A

Which triplets are supported/not supported? Have you updated the CI baseline?
Only try on linux.

Does your PR follow the maintainer guide?
Yes

c72578
c72578 suggested changes May 23, 2020
View reviewed changes
@NancyLi1013 NancyLi1013 self-assigned this May 25, 2020
@myd7349
Copy link
Contributor

myd7349 commented May 25, 2020

This should fix #7280 .

NancyLi1013
NancyLi1013 reviewed May 25, 2020
View reviewed changes
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Does this port only support Linux now?

@NancyLi1013 NancyLi1013 added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response labels May 25, 2020
@curoky
Copy link
Contributor Author

curoky commented May 25, 2020

Does this port only support Linux now?

Yeah, there are some compilation errors on windows. I can’t fix it due to lack of windows environment.

@curoky
Copy link
Contributor Author

curoky commented May 25, 2020

Does this port only support Linux now?

Yeah, there are some compilation errors on windows. I can’t fix it due to lack of windows environment.

And it seems that the official does not support the Windows environment.

@NancyLi1013
Copy link
Contributor

Thanks for the above detailed info.
It seems that it was blocked by thrift on osx.

If so, we need to add Supports: ! windows to the CONTROL file and add vcpkg_fail_port_install(ON_TARGET "windows") to the top of portfile.cmake.

Could you please update this?

NancyLi1013
NancyLi1013 suggested changes May 26, 2020
View reviewed changes
@NancyLi1013
Copy link
Contributor

Please ignore the failure on x64-osx, which is not related with this PR.
I will rerun this PR once it is resolved.

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 17, 2020
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Jun 22, 2020
@NancyLi1013
Copy link
Contributor

@curoky
It seems that you didn't merge the latest commits to this PR. The failures have been solved in mater branch.
Could you please try it again? I try to do this but failed since I have no permission to push the commits to this PR.

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Jun 23, 2020
@NancyLi1013
Copy link
Contributor

Thanks for the update.
LGTM now.

NancyLi1013
NancyLi1013 suggested changes Jul 20, 2020
View reviewed changes
@NancyLi1013 NancyLi1013 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 20, 2020
curoky and others added 4 commits July 24, 2020 07:11
@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 31, 2020
@strega-nil strega-nil merged commit f533327 into microsoft:master Jul 31, 2020
@curoky curoky deleted the add_brpc branch August 16, 2020 17:38
hellozee pushed a commit to hellozee/vcpkg that referenced this pull request Sep 11, 2020
@curoky @NancyLi1013 @hellozee
[brpc] Add new port (microsoft#11524)
d032d76 
* [brpc] add new port

* [brpc] ci ignore build error on windows

* [brpc] update version and delete deprecated function

* [brpc] fail install on windows

Co-authored-by: NancyLi1013 <[email protected]>

* [brpc] not support windows

Co-authored-by: NancyLi1013 <[email protected]>

* Update portfile.cmake

Update the format and remove unused comments

* [brpc] rebase master

* [brpc] reset ci.baseline.txt

Co-authored-by: NancyLi1013 <[email protected]>
@joshuafc
Copy link

Starting package 9/10: brpc:x86-windows
Building package brpc[core]:x86-windows...
CMake Error at scripts/cmake/vcpkg_fail_port_install.cmake:96 (message):
Target 'windows' not supported by brpc!

Call Stack (most recent call first):
ports/brpc/portfile.cmake:1 (vcpkg_fail_port_install)
scripts/ports.cmake:139 (include)

Error: Building package brpc:x86-windows failed with: BUILD_FAILED
Please ensure you're using the latest portfiles with .\vcpkg update, then
submit an issue at https://github.com/Microsoft/vcpkg/issues including:
Package: brpc:x86-windows
Vcpkg version: 2021-05-05-9f849c4c43e50d1b16186ae76681c27b0c1be9d9

@NancyLi1013
Copy link
Contributor

@joshuafc

Thanks for pointing this out. Currently brpc doesn't support windows. As you can see this on upstream, there is still no any plan to support Windows now.
apache/brpc#60
apache/brpc#880

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

@c72578 c72578 c72578 requested changes

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@curoky @myd7349 @NancyLi1013 @JackBoosY @joshuafc @c72578 @strega-nil