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

[gloo] New Port #15219

Merged
merged 6 commits into from Dec 28, 2020
Merged

[gloo] New Port #15219

merged 6 commits into from Dec 28, 2020

Conversation

jacobkahn
Copy link
Contributor

Add a port for gloo.

cc @xw285cornell, @mrshenli, @pbelevich, @osalpekar

cc @syhw, @padentomasello, @tlikhomanenko, @vineelpratap, @xuqiantong, @avidov

  • Which triplets are supported/not supported? Have you updated the CI baseline?

x64-linux, which I've tested. I'm can't tell if there's windows support at this time -- the docs suggest no, but a project maintainer might be able to help me out here. I don't have the means to test it.

Yes

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

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

thanks for your contribution!

Could you add the homepage in vcpkg.json file?

ports/gloo/portfile.cmake Outdated Show resolved Hide resolved
jacobkahn and others added 2 commits December 23, 2020 16:05
Co-authored-by: Phoebe <20694052+PhoebeHui@users.noreply.github.com>
@jacobkahn
Copy link
Contributor Author

One odd thing I can't make work for this port - the contents of usage don't print when installing. I always see

Elapsed time for package gloo:x64-linux: 6.536 s

Total elapsed time: 6.828 s

The package gloo:x64-linux provides CMake targets:

    find_package(Gloo CONFIG REQUIRED)
    target_link_libraries(main PRIVATE gloo)

which seems to be some sort of default. Since the library doesn't provide any imported targets or a config module, I wrote a CMake wrapper which is also in this commit, but I wanted to make the proper usage show up accordingly. @PhoebeHui any idea what I'm doing wrong?

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

@jacobkahn, I noticed gloo provided cmake config and targets file, so you don't need to add the FindGloo.cmake, wrapper and usage file, could you remove them?

BTW, You may need use vcpkg_fixup_cmake_targets to fix up the targets file.

See https://github.com/facebookincubator/gloo/tree/master/cmake
https://github.com/facebookincubator/gloo/blob/master/CMakeLists.txt#L129

@jacobkahn
Copy link
Contributor Author

@PhoebeHui — I didn't even notice that, good catch 👍

Removed the usage, updated, the port, and added vcpkg_fixup_cmake_targets - things seem to be correct now

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your updates!

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 25, 2020
Co-authored-by: Billy O'Neal <bion@microsoft.com>
@BillyONeal BillyONeal merged commit 0a87565 into microsoft:master Dec 28, 2020
@BillyONeal
Copy link
Member

Thanks for your help!

@jacobkahn jacobkahn deleted the gloo branch December 28, 2020 20:28
ryukw7 pushed a commit to ryukw7/vcpkg that referenced this pull request Dec 29, 2020
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.

None yet

3 participants