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

[clipper2] Add new port #29303

Merged
merged 3 commits into from
Feb 3, 2023
Merged

[clipper2] Add new port #29303

merged 3 commits into from
Feb 3, 2023

Conversation

ex-purple
Copy link
Contributor

@ex-purple ex-purple commented Jan 30, 2023 •
edited

Fixes #26448

  • 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 docs/examples/adding-an-explicit-usage.md 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.

github-actions[bot]
github-actions bot previously approved these changes Jan 30, 2023
View reviewed changes
github-actions[bot]
github-actions bot previously approved these changes Jan 30, 2023
View reviewed changes
dg0yt
dg0yt reviewed Jan 31, 2023
View reviewed changes
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.

All possible file conflicts with clipper seem to be resolved by upstream using 2 in file and directory names.
I don't know about symbol names.

@Adela0814 Adela0814 added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response and removed requires:author-response labels Jan 31, 2023
@Adela0814
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Adela0814
Copy link
Contributor

I got a bad hash error in my local:

error: Failed to download from mirror set
error: File does not have the expected hash:
url: https://github.com/AngusJohnson/Clipper2/archive/Clipper2_1.1.1.tar.gz
File: E:\3\vcpkg\downloads\AngusJohnson-Clipper2-Clipper2_1.1.1.tar.gz.936.part
Expected hash: b2b35cf4d03a387f43ee65ea49a30ec194d75ecf1b9b7431263c9073ee35ac63419ae3714f15220e5865e437b2bb5d9863cc01c5a3844304bb61933ae8c03c5b
Actual hash: e996ef8a2ec412189f0ba95a6f200c0818f9755930b05cd20b630d33760dec619c6a735ac056f5dfbaccf793bf6ebeb7b6c102fd9ff83b0d297a4d660389d8d9

@ex-purple
Copy link
Contributor Author

@Adela0814

I got a bad hash error in my local

I cannot help with this. For me and Azure Pipelines, it does not reproduce.

@ex-purple
Copy link
Contributor Author

@dg0yt
Clipper and Clipper2 looks very difference:
https://sourceforge.net/projects/polyclipping/
https://sourceforge.net/p/polyclipping/code/HEAD/tree/tags/6.2.0/cpp/CMakeLists.txt

I don't think that we should change something in the typical Clipper2 installation

@dg0yt
Copy link
Contributor

dg0yt commented Jan 31, 2023

@Adela0814

I got a bad hash error in my local

I cannot help with this. For me and Azure Pipelines, it does not reproduce.

@ex-purple Maybe you local downloads folder (and vcpkg CI) has the asset with the SHA512 from GH yesterday's temporary change?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 31, 2023 •
edited

@dg0yt Clipper and Clipper2 looks very difference: https://sourceforge.net/projects/polyclipping/ https://sourceforge.net/p/polyclipping/code/HEAD/tree/tags/6.2.0/cpp/CMakeLists.txt

I don't think that we should change something in the typical Clipper2 installation

I didn't ask for changes. I just wanted to avoid surprises if both libaries end up in the linking of a single executable.

The answer is : Clipper2 uses a different C++ namespace. So really no problem.

dg0yt
dg0yt reviewed Jan 31, 2023
View reviewed changes
ports/clipper2/portfile.cmake
@@ -0,0 +1,25 @@
if(VCPKG_TARGET_IS_WINDOWS)
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see an unguarded

#define EXTERN_DLL_EXPORT extern "C" __declspec(dllexport)

in CPP/Clipper2Lib/include/clipper2/clipper.export.h.

I guess for dynamic linkage support, this must be replaced in the installed header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have never experienced DDL export in practice, but I can try this option

Copy link
Contributor

Choose a reason for hiding this comment

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

We may change it in an update if you don't feel comfortable with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please 🙏

@ex-purple
Copy link
Contributor Author

ex-purple commented Jan 31, 2023 •
edited

The answer is : Clipper2 uses a different C++ namespace. So no really no problem.

Thanks for the lesson. I'm not native speaker 🙂

@ex-purple Maybe you local downloads folder (and vcpkg CI) has the asset with the SHA512 from GH yesterday's temporary change?

Maybe
Could you tell me what kind of changes I should make?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 31, 2023

Could you tell me what kind of changes I should make?

Delete your local downloads\AngusJohnson-Clipper2-Clipper2_1.1.1.tar.gz, then rebuild. You will see that the hash changed.
(Yesterday a change of hashes at GH broke many existing recipes. Your recipe is broken because that change was rolled back. Cf. #29288)

@ex-purple
Copy link
Contributor Author

ex-purple commented Jan 31, 2023 •
edited

Delete your local downloads\AngusJohnson-Clipper2-Clipper2_1.1.1.tar.gz, then rebuild. You will see that the hash changed.

Indeed, I updated actual hash

@BillyONeal
Copy link
Member

I manually checked that the SHA in here is not affected by #29288

@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Feb 2, 2023
@BillyONeal BillyONeal merged commit bb9766f into microsoft:master Feb 3, 2023
@BillyONeal
Copy link
Member

Thanks for the new port!

@dg0yt Thanks for your review!

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

@dg0yt dg0yt dg0yt left review comments

@github-actions github-actions[bot] github-actions approved these changes

@Adela0814 Adela0814 Adela0814 approved these changes

Assignees

@Adela0814 Adela0814

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.

[New Port Request] Clipper2
4 participants
@ex-purple @Adela0814 @dg0yt @BillyONeal