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

[rsocket] Add new port #11021

Merged
merged 12 commits into from
Jul 31, 2020
Merged

[rsocket] Add new port #11021

merged 12 commits into from
Jul 31, 2020

Conversation

curoky
Copy link
Contributor

@curoky curoky commented Apr 25, 2020

Describe the pull request
C++ implementation of RSocket http://rsocket.io
https://github.com/rsocket/rsocket-cpp

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

Which triplets are supported/not supported? Have you updated the CI baseline?
Not support x64-windows x64-windows-static

Does your PR follow the maintainer guide?
Yes

ports/rsocket/CONTROL Outdated Show resolved Hide resolved
ports/rsocket/CONTROL Outdated Show resolved Hide resolved
ports/rsocket/portfile.cmake Outdated Show resolved Hide resolved
ports/rsocket/portfile.cmake Outdated Show resolved Hide resolved
@@ -1587,6 +1587,8 @@ rpclib:x64-uwp=fail
rpclib:x64-windows=ignore
rpclib:x86-windows=ignore
rpclib:x64-windows-static=ignore
rsocket:x64-windows=fail
Copy link
Contributor

Choose a reason for hiding this comment

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

If this port does not support x64-windows and x64-windows-static, please add vcpkg_fail_port_install(ON_ARCH "x64") to the top of portfile.cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

@curoky ,
folly itself according to it's port file supports x64 only (there is restriction in port file) and just mention instability on Windows in CONTROL file. Why not just rely on folly as dependency. If it's available then rsocket-cpp will work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@curoky , I cannot see explanation of the feature of the CONTROL file ( https://vcpkg.readthedocs.io/en/latest/maintainers/control-files/ )

but folly has
in folly itself I see following in CONTROL file
Supports: x64

In other port I see following (I assume this is some kind of restrictions) which can be usefull

Supports: (windows|linux)&x64
Supports: x86 | x64
Supports: !(windows|arm|uwp)

Copy link
Contributor

Choose a reason for hiding this comment

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

@curoky ,
according to folly readme windows is actually supported

folly supports gcc (5.1+), clang, or MSVC. It should run on Linux (x86-32, x86-64, and ARM), iOS, macOS, and Windows (x86-64).

I've found related issues

I suggest to not restrict platform for rsocket. Just require that folly exists.

ports/rsocket/portfile.cmake Show resolved Hide resolved
@curoky
Copy link
Contributor Author

curoky commented Apr 26, 2020

Maybe I shouldn't add vcpkg_fail_port_install(ON_ARCH "x64"), it seems that x64 contains linux environment.
I just want skip windows environment.

ports/rsocket/CONTROL Outdated Show resolved Hide resolved
ports/rsocket/portfile.cmake Outdated Show resolved Hide resolved
@@ -0,0 +1,26 @@
diff --git a/cmake/rsocket-config.cmake.in b/cmake/rsocket-config.cmake.in
Copy link
Contributor

Choose a reason for hiding this comment

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

@curoky , can you please explain the purpose of this patch? I've managed to build (at least) latest master under vcpkg without patching. Is there any issues with using lib in target project?

Copy link
Contributor

Choose a reason for hiding this comment

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

@curoky ,
I've got why this is needed. Maybe it worth to propose such patch to rsocket-cpp itself.

@curoky
Copy link
Contributor Author

curoky commented May 1, 2020

Thanks for your suggestions @malirod .
The main reason why I compiled this package is that fbthrift depends on this, so all changes are for compiling fbthrift.
I will try my best to solve the above problem. However, I cannot guarantee that the windows can be used since there is no windows environment.

@malirod
Copy link
Contributor

malirod commented May 3, 2020

@curoky ,
seems folly built fine on linux. there is something wrong with rsocket itself
https://dev.azure.com/vcpkg/c1ee48cb-0df2-4ab3-8384-b1df5a79fe53/_apis/build/builds/35961/logs/58

2020-05-02T10:07:45.2960882Z Installing package folly[core,zlib]:x64-windows...
2020-05-02T10:07:45.2961217Z Installing package folly[core,zlib]:x64-windows... done
2020-05-02T10:07:45.2961481Z Elapsed time for package folly:x64-windows: 10.21 s
2020-05-02T10:07:45.2962094Z Starting package 84/84: rsocket:x64-windows
2020-05-02T10:07:45.2962603Z Building package rsocket[core]:x64-windows...
2020-05-02T10:07:45.2963220Z Could not locate cached archive: C:\agent_work\1\s\archives\77\779727562299ee2c6b7f57c07b2704e69734c3d8.zip
2020-05-02T10:08:07.7616776Z -- Downloading https://github.com/rsocket/rsocket-cpp/archive/b237f5dba44bd360ee8c24fb998af83606355116.tar.gz...
2020-05-02T10:08:07.7617845Z -- Extracting source C:/agent/_work/1/s/downloads/rsocket-rsocket-cpp-b237f5dba44bd360ee8c24fb998af83606355116.tar.gz
2020-05-02T10:08:07.7618398Z -- Applying patch fix-cmake-config.patch
2020-05-02T10:08:07.7618746Z -- Using source at C:/agent/_work/1/s/buildtrees/rsocket/src/3606355116-b3d329c9e2
2020-05-02T10:08:07.7619220Z -- Downloading https://github.com/ninja-build/ninja/releases/download/v1.10.0/ninja-win.zip...
2020-05-02T10:08:07.7620321Z -- Configuring x64-windows
2020-05-02T10:08:07.7620826Z CMake Error at scripts/cmake/vcpkg_execute_required_process.cmake:72 (message):
2020-05-02T10:08:07.7621571Z Command failed: ninja -v
2020-05-02T10:08:07.7622458Z Working Directory: C:/agent/_work/1/s/buildtrees/rsocket/x64-windows-rel/vcpkg-parallel-configure
2020-05-02T10:08:07.7622859Z Error code: 1
2020-05-02T10:08:07.7623647Z See logs for more information:
2020-05-02T10:08:07.7624559Z C:\agent_work\1\s\buildtrees\rsocket\config-x64-windows-out.log
2020-05-02T10:08:07.7624845Z
2020-05-02T10:08:07.7625765Z Call Stack (most recent call first):
2020-05-02T10:08:07.7626407Z scripts/cmake/vcpkg_configure_cmake.cmake:297 (vcpkg_execute_required_process)
2020-05-02T10:08:07.7627151Z ports/rsocket/portfile.cmake:16 (vcpkg_configure_cmake)
2020-05-02T10:08:07.7627972Z scripts/ports.cmake:90 (include)
2020-05-02T10:08:07.7628604Z
2020-05-02T10:08:07.7629298Z
2020-05-02T10:08:07.8966038Z Error: Building package rsocket:x64-windows failed with: BUILD_FAILED
2020-05-02T10:08:07.8966823Z Elapsed time for package rsocket:x64-windows: 22.6 s

@curoky
Copy link
Contributor Author

curoky commented May 4, 2020

@malirod may need to add the following lines

  1. find_package(ZLIB MODULE REQUIRED)
  2. target_link_libraries(ReactiveSocket ... ZLIB::ZLIB)
  3. target_link_libraries(yarpl ... ZLIB::ZLIB)

Do you have a windows environment? Try it?

-- Found folly: C:/agent/_work/1/s/installed/x64-windows-static
yarpl source dir: C:/agent/_work/1/s/buildtrees/rsocket/src/3606355116-b3d329c9e2/yarpl
-- Configuring done

CMake Error at C:/agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:250 (_add_library):
  Target "ReactiveSocket" links to target "ZLIB::ZLIB" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:205 (add_library)

CMake Error at C:/agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:250 (_add_library):
  Target "yarpl" links to target "ZLIB::ZLIB" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  yarpl/CMakeLists.txt:67 (add_library)

@malirod
Copy link
Contributor

malirod commented May 5, 2020

@curoky ,
I have win env and I was able to reproduce build issue you've posted. I've added lines you've suggested and managed to pass configuration step. Then there were build issues (see below).
I've playing with the following branch ( https://github.com/malirod/vcpkg/tree/add-port-rsocket-cpp )

Win x63 build issues

  1. Command line warning D9025 : overriding '/std:c++latest' with '/std:c++14'
  2. vcpkg\installed\x64-windows\include\folly/container/detail/F14Mask.h(41): error C3861: '__builtin_ctzll': identifier not found
  3. vcpkg\installed\x64-windows\include\folly/lang/Bits.h(147): error C3861: '__builtin_clzl': identifier not found

Additionally I found following things and propose to add them to this PR

  1. There are recent commits in the rsocket-cpp repo and I see tags so it's possible to set proper version of the port
Version: v2020.05.04.00
  1. Fmt is not mentioned in readme file but it's required in rsocket and yarpl cmake file. So It's required to mention this in CONTROL file.
    Build-Depends: folly, fmt, zlib

3.Latest commit and sha

REF 8038d05e741c3d3ecd6adb069b4a1b3daa230e14
    SHA512 d7bc93af7b6130d73fa0823f534ad57a531dfa7d7aa990a2a1a1b72b6761db7eeb60573d0d38f55daa991554e3ab4ac507047f8051a4390b3343cd708a48efbb

Would be great to get feedback about these items.

I'll continue to try to make win build for a while. If you have any ideas how to fix issues please suggest.

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

If this port doesn't support x64 triplets.
Please add the following codes to VCPKG_PATH/scripts/ci.baseline.txt:

rsocket:x64-windows=fail
rsocket:x64-windows-static=fail

ports/rsocket/portfile.cmake Outdated Show resolved Hide resolved
ports/rsocket/CONTROL Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PhoebeHui
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor

When used with Visual Studio 2019, folly seems to have a bug.

@JackBoosY
Copy link
Contributor

Waiting for #11370 merge and rerun pipeline test.

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 9, 2020
@JackBoosY
Copy link
Contributor

When builiding rsocket with x64-windows:

E:\install\x64-windows\include\folly/container/detail/F14Mask.h(39): error C3861: '__builtin_ctz': identifier not found

ports/rsocket/portfile.cmake Show resolved Hide resolved
ports/rsocket/CONTROL Show resolved Hide resolved
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 22, 2020
@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil strega-nil merged commit 62cbf26 into microsoft:master Jul 31, 2020
@curoky curoky deleted the add_rsocket branch August 16, 2020 17:38
hellozee pushed a commit to hellozee/vcpkg that referenced this pull request Sep 11, 2020
* [rsocket] Add new port

* [rsocket] don't build in some window environment

* [rsocket] add missing deps in CONTROL

* [rsocket] add missing deps in CONTROL

* [rsocket] remove some deprecated args

* [rsocket] romove vcpkg_fail_port_install(ON_ARCH x64)

* [rsocket] only support x64 architecture

* [rsocket] upgrade to 2020.05.04.00

* [rsocket] add vcpkg_fail_port_install to portfile

* [rsocket] Fix find dependencies

* Update ports/rsocket/portfile.cmake

* Update ports/rsocket/CONTROL

Co-authored-by: JackBoosY <yuzaiyang@beyondsoft.com>
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

[New Port Request] rsocket-cpp
5 participants