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

[sese] add new port #38704

Merged
merged 15 commits into from
Jun 8, 2024
Merged

[sese] add new port #38704

merged 15 commits into from
Jun 8, 2024

Conversation

SHIINASAMA
Copy link
Contributor

  • 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.
  • 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.

@SHIINASAMA
Copy link
Contributor Author

@SHIINASAMA请阅读以下贡献者许可协议(CLA)。如果您同意 CLA,请回复以下信息。

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

选项:

  • (默认 - 未指定公司)我对我提交的内容拥有知识产权的唯一所有权,并且我不会在为我的雇主工作的过程中提交内容。
@microsoft-github-policy-service agree
  • (当公司提供时)我在为我的雇主工作的过程中提交意见(或者我的雇主根据合同或适用法律对我提交的意见拥有知识产权)。我已获得雇主的许可,可以代表我的雇主提交意见并签订本协议。通过在下面签名,定义的术语“您”包括我和我的雇主。
@microsoft-github-policy-service agree company="Microsoft"

贡献者许可协议

贡献许可协议

本贡献许可协议(**“协议” )经下面签署方( **“您” )同意, 并向 Microsoft Corporation 及其附属公司(“Microsoft”)转让您 对 Microsoft 开源项目的贡献的某些许可权利。本协议自以下最新签署日期起生效 。

  1. 定义
    **“代码”**是指
    您根据本协议向 Microsoft 交付的计算机软件代码,无论是人类可读的形式还是机器可执行的形式。
    **“项目”**是指由 Microsoft 拥有或管理并根据
    开源促进会 ( www.opensource.org ) 批准的许可证提供的任何项目。
    **“提交”**是指向任何项目上传、提交、传输或分发代码或其他内容的行为
    ,包括但不限于在由其管理或在其上管理的电子邮件列表、源代码控制
    系统和问题跟踪系统上进行通信。代表本项目,出于讨论和改进该项目的目的,但不包括明显标记或
    由您以其他书面方式指定为“非提交内容”
    的通信。 **“提交内容”**是指您提交的代码和任何其他受版权保护的材料,包括任何
    相关的评论和文档。
  2. 您所提交。在提交任何
    项目之前,您必须同意本协议的条款。本协议涵盖您现在或将来(
    下文第 4 节中所述的情况除外)向任何项目提交的任何及所有提交内容。
  3. 作品的原创性。您声明您提交的每份内容完全是您的原创作品。
    如果您希望提交非您原创作品的材料,您可以将它们单独提交
    给项目,前提是您 (a) 保留您收到材料时包含的所有版权和许可信息
    ,(b) 在您的材料随附的说明中提交内容包括短语“
    包含第三方材料的提交内容”:后跟第三方名称以及
    您所知的任何许可或其他限制,并且 (c) 遵循项目
    关于提交内容的书面指南中的任何其他说明。
  4. 你的雇主。本协议中提及的“雇主”包括您的雇主或
    您代表其提交意见的任何其他人,例如承包商、供应商或代理人。如果您的
    提交内容是在为雇主工作的过程中提交的,或者您的雇主
    根据合同或适用法律对您提交的内容拥有知识产权,则您必须
    在签署本协议之前获得雇主的许可才能提交该提交内容。在这种情况下,本协议中的“您”一词
    将统指您和雇主。如果您将来更换雇主并
    希望为新雇主提交额外的提交材料,则您同意在提交这些提交材料之前签署新协议
    并获得新雇主的许可。
  5. 许可证
  • 版权许可。您向 Microsoft 以及直接或间接从 Microsoft 接收提交内容的人员授予
    永久、全球性、非排他性、免版税、不可撤销的提交内容许可,
    以复制、准备衍生作品、公开展示、公开表演和分发
    提交内容和此类衍生作品,并将任何或所有上述权利再许可给第三方
  • 专利许可。您根据您的专利声明向 Microsoft 以及直接或间接从 Microsoft 接收提交内容的人员授予
    永久、全球性、非独占、免版税、不可撤销的许可,而提交内容或
    提交内容与提交内容
    的组合必然会侵犯您的专利权利要求。提交的项目将
    单独或与项目一起制作、已经制作、使用、要约出售、出售和进口或以其他方式处置提交内容。
  • 保留其他权利。各方保留本协议中未明确授予的所有权利。不
    以暗示、用尽、禁止反言或其他方式授予
    任何其他许可或权利(包括但不限于任何默示许可) 。
  1. 陈述和保证。您声明您具有授予上述
    许可的合法权利。您声明您提交的每项内容均完全是您的原创作品(您
    根据第 3 条可能披露的情况除外)。您声明
    ,如果您的提交内容是在您为雇主工作的过程中提交的,或者您的雇主根据合同或适用
    法律
    对您提交的内容拥有知识产权,则您已获得雇主的许可进行提交。如果您代表您的雇主签署本协议,则您声明并保证您
    拥有必要的权力来约束所列雇主遵守本协议中包含的义务。
    除非您选择这样做,否则您不需要为您提交的内容提供支持。除非适用法律要求或书面同意,并且除
    第 3、4 和 6 条中明确规定的
    保证外,根据本协议提供的提交内容
    不提供任何类型的保证,包括但不限于任何保证非
    侵权、适销性或特定用途的适用性。
  2. 通知微软.您同意以书面形式通知微软
    您后来发现的任何可能导致您在本协议中的陈述在任何
    方面不准确的事实或情况。
  3. 有关提交的信息。您同意对项目的贡献和有关
    贡献的信息可以无限期保留并公开披露,包括您的姓名和
    您随提交内容一起提交的其他信息。
  4. 适用法律/司法管辖区。本协议受华盛顿州法律管辖,
    双方同意专属管辖权并在位于
    华盛顿州金县的联邦法院进行审判,除非不存在联邦主题管辖权,在这种情况下,双方同意
    专属管辖权审判地点为华盛顿州金县高等法院。双方放弃所有
    因缺乏属人管辖权和法院不方便而进行的抗辩。
  5. 整个协议/转让。本协议是双方之间的完整协议,并取代
    双方之间与本协议主题相关的
    任何及所有先前的书面或口头协议、谅解或通信。本协议可由 Microsoft 转让。

@microsoft-github-policy-service agree

@SHIINASAMA SHIINASAMA marked this pull request as ready for review May 12, 2024 15:14
@SHIINASAMA
Copy link
Contributor Author

I'm using the add new port function for the first time and it looks like some of the checks are not passing.

What should I do?🙃

@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label May 13, 2024
ports/sese/portfile.cmake Outdated Show resolved Hide resolved
ports/sese/portfile.cmake Outdated Show resolved Hide resolved
@jimwang118
Copy link
Contributor

When the async-logger feature is enabled, the following error will occur.
F:\sese\buildtrees\sese\src\2.1.1-9ed8adafac.clean\sese\record\Logger.cpp(93): error C2061: syntax error: identifier 'AsyncLogger'

@jimwang118 jimwang118 marked this pull request as draft May 13, 2024 03:05
@SHIINASAMA
Copy link
Contributor Author

I fixed the missing symbols when enabling async logger, and finished the generic function to remove empty directories, which all validated on my private registry, so I think it's probably time for a review.

@SHIINASAMA SHIINASAMA marked this pull request as ready for review May 13, 2024 04:40
ports/sese/usage Outdated Show resolved Hide resolved
ports/sese/portfile.cmake Outdated Show resolved Hide resolved
ports/sese/portfile.cmake Outdated Show resolved Hide resolved
ports/sese/portfile.cmake Outdated Show resolved Hide resolved
ports/sese/vcpkg.json Outdated Show resolved Hide resolved
@jimwang118 jimwang118 marked this pull request as draft May 13, 2024 06:16
jimwang118
jimwang118 previously approved these changes May 13, 2024
@SHIINASAMA SHIINASAMA marked this pull request as ready for review May 13, 2024 10:31
@jimwang118
Copy link
Contributor

Compile test pass with following triplets:

x64-windows

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label May 14, 2024
"description": "a cross-platform framework",
"homepage": "https://libsese.github.io/sese/",
"license": "Apache-2.0",
"supports": "x64 & (windows | osx | linux) & !uwp & !static",
Copy link
Contributor

Choose a reason for hiding this comment

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

This line makes the port untested in all static triplets, including ALL of linux and osx.
What prevents building a static lib in linux?
A cross-platform framework which can be tested only on one platform doesn't look attractive.

{
"name": "sese",
"version": "2.1.1",
"description": "a cross-platform framework",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "a cross-platform framework",
"description": "A cross-platform framework",

Not very descriptive, and the homepage doesn't help at all due to language barriers.

@jimwang118 jimwang118 removed the info:reviewed Pull Request changes follow basic guidelines label May 14, 2024
@SHIINASAMA
Copy link
Contributor Author

@dg0yt Thanks for your reviews.

  • The home page field actually points to the full document path, and we don't have enough people to internationalize it at the moment. Maybe I can point it to the warehouse address, and the README might be more valuable than the default document home page? I'm not sure. There are also plans to rewrite README recently.
  • Regarding the testing on different platforms, the code itself compiles across all three platforms (in the repository's github actions) and I've tested it correctly in our private registry. If I may add, I lack osx-related test equipment on hand, so I may only be able to complete port testing for two of the platforms. (PS: Using it through the private registry is fine when my x64 osx VM is still booting properly)
  • There are two considerations for not supporting static links. I am concerned that static links may have copyright issues; Static linking under unix-like, cmake requires the user to explicitly call all dependent find_packages, which is not what I expected (even though I adjusted the visibility of the dependencies).
  • As you can see from the third point, the triples actually supported are x64-windows, x64-linux-dynamic, and x64-osx-dynamic

@dg0yt
Copy link
Contributor

dg0yt commented May 15, 2024

  • There are two considerations for not supporting static links. I am concerned that static links may have copyright issues;

IANAL but I don't think this is a valid point here. I don't even want to go into details. If users are unsure, they must contact a lawyer.

Static linking under unix-like, cmake requires the user to explicitly call all dependent find_packages, which is not what I expected (even though I adjusted the visibility of the dependencies).

No. Your exported cmake config must call find_dependency for all imported targets it links to, for static linkage. Basic cmake stuff.

You announce cross-platform and vcpkg support so this should really be fixed. If someone adds reverse dependencies on sese to vcpkg, you want it to be tested on all supported triplets.

@SHIINASAMA SHIINASAMA marked this pull request as draft May 15, 2024 10:09
@SHIINASAMA
Copy link
Contributor Author

You're right, I shouldn't be thinking about these things for the user. Ports now support static linking.

And I also tested the port on the triplet corresponding platform in the workflow of the project.

image

@SHIINASAMA SHIINASAMA marked this pull request as ready for review May 17, 2024 06:38
@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label May 17, 2024
@vicroms
Copy link
Member

vicroms commented May 29, 2024

Hi @SHIINASAMA

We are considering new acceptance criteria for ports in the vcpkg registry.

Given that this is a new library with no pre-existing user base and it’s not part of a well stablished suite of libraries, we believe that it is a good candidate for self-hosting in a custom registry. Users will still be able to install your library with vcpkg using either the registries or overlay ports features; and you get full control of when you update the port and the changes it includes.

Would you be interested in setting up a custom registry for your library? We can aid setting up your registry and providing instructions for your users to install your vcpkg port.

We are also interested in receiving feedback regarding the suggested alternative if you decide to give it a try.

I'm leaving this PR as a draft to serve as a way of communication with you regarding the suggested alternative of self-hosting this port. Feel free to use this thread to request assistance with said task.

@vicroms vicroms marked this pull request as draft May 29, 2024 08:11
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label May 29, 2024
@vicroms
Copy link
Member

vicroms commented May 29, 2024

I see that you already provide instructions to install your package via a registry. You could expand your instructions for users that prefer to install your packages via overlay ports.

You can install this package with vcpkg as an overlay port using these steps:

git clone https://github.com/libsese/vcpkg-registry
$env:VCPKG_OVERLAY_PORTS=C:/path/to/libsese-vcpkg-registry/ports;$env:VCPKG_OVERLAY_PORTS"
vcpkg install sese

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

@SHIINASAMA

We decided to reverse our new port acceptance criteria policy. Please address the PR comments so we can accept this port into the vcpkg registry.

"features": [
"openssl"
],
"version>=": "1.29.0"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all version>= constraints from the manifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related fields have been removed

@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 7, 2024
update `2.1.1` to `2.1.2`
@SHIINASAMA SHIINASAMA marked this pull request as ready for review June 8, 2024 03:59
@vicroms vicroms merged commit 14b9179 into microsoft:master Jun 8, 2024
17 checks passed
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.

4 participants