Skip to content

Conversation

171930433
Copy link
Contributor

No description provided.

@jimwang118 jimwang118 changed the title Gsk/add yalantinglibs [yalantinglibs] Add a new port Nov 15, 2024
@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 15, 2024
@jimwang118
Copy link
Contributor

I will convert this PR to a draft. After all CIs are green, you can click "ready for review".

@jimwang118 jimwang118 marked this pull request as draft November 15, 2024 02:04
171930433 added a commit to 171930433/yalantinglibs that referenced this pull request Nov 18, 2024
@171930433 171930433 closed this Nov 18, 2024
@171930433 171930433 reopened this Nov 18, 2024
@171930433
Copy link
Contributor Author

@171930433 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@171930433
Copy link
Contributor Author

@microsoft-github-policy-service agree

@171930433
Copy link
Contributor Author

I have already replied to the CLA, but why did all the CIs fail?
@jimwang118 @dg0yt

@dg0yt
Copy link
Contributor

dg0yt commented Nov 18, 2024

why did all the CIs fail?

Looks like a temporary connectivity issue.

Let me add some comments.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

FTR upstream has zero external usage/stars/contributors.

"name": "yalantinglibs",
"version": "0.3.9",
"homepage": "https://github.com/171930433/yalantinglibs",
"description": "A Collection of C++20 libraries, include struct_pack, struct_json, struct_xml, struct_yaml, struct_pb, easylog, coro_rpc, coro_http and async_simple",
Copy link
Contributor

Choose a reason for hiding this comment

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

This description is a vague description of what users could expect.

@171930433
Copy link
Contributor Author

Platform Android is not yet supported by yalantinglibs . Could you please help me check why the x86_windows build failed? @dg0yt

@171930433 171930433 marked this pull request as ready for review November 20, 2024 01:12
@poor-circle
Copy link
Contributor

the dependence is loss, asio, frozen and
async_simple should add

@jimwang118
Copy link
Contributor

Usage test passed with x64-windows triplet.

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Nov 20, 2024
@poor-circle
Copy link
Contributor

we need add option INSTALL_THIRDPARTY=OFF
, and install them by vcpkg

@171930433
Copy link
Contributor Author

we need add option INSTALL_THIRDPARTY=OFF , and install them by vcpkg

目前yalantinglibs在vcpkg这边使用的还是我fork的仓库,因为需要alibaba/yalantinglibs打一个tag,

  1. “the dependence is loss, asio, frozen and async_simple should add”, 这个没太理解我需要怎么操作
  2. “we need add option INSTALL_THIRDPARTY=OFF , and install them by vcpkg”, 这个明确

这些改动可以等到alibaba/yalantinglibs打一个tag时, 一起修改

@jimwang118 jimwang118 removed the info:reviewed Pull Request changes follow basic guidelines label Nov 20, 2024
@poor-circle
Copy link
Contributor

  1. 这个没太理解我需要怎么操作

we need add option INSTALL_THIRDPARTY=OFF , and install them by vcpkg

目前yalantinglibs在vcpkg这边使用的还是我fork的仓库,因为需要alibaba/yalantinglibs打一个tag,

  1. “the dependence is loss, asio, frozen and async_simple should add”, 这个没太理解我需要怎么操作
  2. “we need add option INSTALL_THIRDPARTY=OFF , and install them by vcpkg”, 这个明确

这些改动可以等到alibaba/yalantinglibs打一个tag时, 一起修改

你看下我的review,tag也加了,最新版本0.3.10,你可以切到这个版本。

@BillyONeal
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@171930433
Copy link
Contributor Author

@poor-circle @dg0yt @jimwang118 all ci passed

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Nov 22, 2024
@BillyONeal
Copy link
Member

  • Changes comply with the maintainer guide.

  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.

  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.

    There appear to be a number of unguarded optional dependencies here:

    src\easylog\benchmark\CMakeLists.txt

    find_package(glog QUIET)
    find_package(spdlog QUIET)
    

    src\struct_pack\benchmark\CMakeLists.txt

    find_package(Protobuf QUIET)
    find_package(Flatbuffers QUIET)
    

    These are probably fine because tests are turned off:

    src\coro_http\tests\CMakeLists.txt

    find_package(ZLIB)
    

    src\struct_pb\tests\CMakeLists.txt

    find_package(Protobuf QUIET)
    

    These are probably fine because examples are turned off:

    src\coro_rpc\examples\user_defined_rpc_protocol\rest_rpc\CMakeLists.txt

    find_package(Msgpack)
    
  • The versioning scheme in vcpkg.json matches what upstream says.

  • The license declaration in vcpkg.json matches what upstream says.

  • The installed as the "copyright" file matches what upstream says.

  • The source code of the component installed comes from an authoritative source.

  • The generated "usage text" is accurate. See adding-usage for context.

  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.

  • Only one version is in the new port's versions file.

  • Only one version is added to each modified port's versions file.

@BillyONeal
Copy link
Member

Can you describe why the potentially optional dependencies are missing above, or fix them to be explicitly disabled or included in vcpkg.json? Then un-draft this PR so we look at it again.

Thanks for your contribution to vcpkg!

@BillyONeal BillyONeal marked this pull request as draft November 22, 2024 22:49
@poor-circle
Copy link
Contributor

poor-circle commented Nov 23, 2024

Can you describe why the potentially optional dependencies are missing above, or fix them to be explicitly disabled or included in vcpkg.json? Then un-draft this PR so we look at it again.

Thanks for your contribution to vcpkg!

this library will automatic install denpendecies by itself, so i suggest add option to disable it and install denpendecies by vcpkg. they are not optional denpendecies.

@171930433 171930433 marked this pull request as ready for review November 23, 2024 12:38
@BillyONeal
Copy link
Member

this library will automatic install denpendecies by itself

That is not allowed for ports in our curated registry: https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#do-not-use-vendored-dependencies

so i suggest add option to disable it and install denpendecies by vcpkg. they are not optional denpendecies.

Then I think they need to be in the "dependencies" block in vcpkg.json?

@BillyONeal BillyONeal marked this pull request as draft November 25, 2024 20:34
@171930433
Copy link
Contributor Author

this library will automatic install denpendecies by itself

That is not allowed for ports in our curated registry: https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#do-not-use-vendored-dependencies

so i suggest add option to disable it and install denpendecies by vcpkg. they are not optional denpendecies.

Then I think they need to be in the "dependencies" block in vcpkg.json?

  1. The glog, spdlog, Protobuf and Flatbuffers libraries are only used during testing or benchmarking.
  2. I have already added the vcpkg_cmake_configure with " -DBUILD_BENCHMARK=OFF -DBUILD_EXAMPLES=OFF -
    BUILD_UNIT_TESTS=OFF -DINSTALL_THIRDPARTY=OFF ". The tests and benchmarks will be closed
  3. We also add the vcpkg_cmake_configure with -DINSTALL_THIRDPARTY=OFF. This is the option to allow install dependencies by vcpkg

@171930433 171930433 marked this pull request as ready for review November 26, 2024 02:01
@BillyONeal
Copy link
Member

BillyONeal commented Nov 26, 2024

I have already added the vcpkg_cmake_configure with " -DBUILD_BENCHMARK=OFF -DBUILD_EXAMPLES=OFF -
BUILD_UNIT_TESTS=OFF -DINSTALL_THIRDPARTY=OFF ". The tests and benchmarks will be closed

I see, I missed the BUILD_BENCHMARK one. Thanks!

@BillyONeal BillyONeal merged commit 41800ce into microsoft:master Nov 26, 2024
17 checks passed
@171930433
Copy link
Contributor Author

171930433 commented Nov 27, 2024

I have already added the vcpkg_cmake_configure with " -DBUILD_BENCHMARK=OFF -DBUILD_EXAMPLES=OFF -
BUILD_UNIT_TESTS=OFF -DINSTALL_THIRDPARTY=OFF ". The tests and benchmarks will be closed

I see, I missed the BUILD_BENCHMARK one. Thanks!

By the way, how to add the introduction of the yalantinglibs library? vcpkg query @BillyONeal

@171930433
Copy link
Contributor Author

I tested the latest master branch and got an error message with "error: the baseline does not contain an entry for port yalantinglibs" @jimwang118 @dg0yt @BillyONeal Please teach me how to solve this problem.

@jimwang118
Copy link
Contributor

I tested the latest master branch and got an error message with "error: the baseline does not contain an entry for port yalantinglibs" @jimwang118 @dg0yt @BillyONeal Please teach me how to solve this problem.

This means that the code you downloaded does not contain yalantinglibs port. You can check whether there is a yalantinglibs folder in the vcpkg/ports directory. If not, update the vcpkg library.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 27, 2024

This is a manifest mode message. IIUC you need to update not only the vcpkg checkout but also the project's chosen vcpkg baseline.

@171930433
Copy link
Contributor Author

This is a manifest mode message. IIUC you need to update not only the vcpkg checkout but also the project's chosen vcpkg baseline.

Thanks. I modified the "default-registry"-"baseline" under "vcpkg-configuration.json" to the latest "master", and the problem has been solved.

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.

5 participants