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

[polyclipping] new port #6636

Merged
merged 3 commits into from
Jun 3, 2019
Merged

Conversation

cenit
Copy link
Contributor

@cenit cenit commented May 27, 2019

First part of a series, to resolve vxt regression related to openjpeg, which I cannot stand anymore 😆

@Neumann-A
Copy link
Contributor

Neumann-A commented May 27, 2019

You probably want to patch the CMakeLists.txt of that port.

It has something like:
SET(CMAKE_BUILD_TYPE "Release" CACHE STRING "Release type")

@cenit
Copy link
Contributor Author

cenit commented May 27, 2019

may I ask you why should I patch SET(CMAKE_BUILD_TYPE "Release" CACHE STRING "Release type")?
Also I can't understand the other suggestion about INSTALL...

There are no config files produced by this build, it is common to have your own FindClipper.cmake to find this library, so also the vcpkg_fixup_cmake_targets() is not necessary

@Neumann-A
Copy link
Contributor

Neumann-A commented May 27, 2019

There are no config files produced by this build, it is common to have your own FindClipper.cmake to find this library, so also the vcpkg_fixup_cmake_targets() is not necessary

sry, I delete that sentence seconds after I posted it.

may I ask you why should I patch SET(CMAKE_BUILD_TYPE "Release" CACHE STRING "Release type")?

because it overwrites the build type or does it not? Argh CMakes (stupid) syntax rules. set(bla CACHE) only sets the value if it has not been supplied. Sry for that, was a bit too fast.

own FindClipper.cmake

I find that a bit strange because it installs a package configuration file. Although it has not a lot of content.

The port should probably be renamed to polyclipping because there are a lot of other packages called clipper and the first one on google with that name is actually not this port but this: https://github.com/ucbrise/clipper . Furthermore the project name in the CMakelists is polyclipping

@cenit
Copy link
Contributor Author

cenit commented May 27, 2019

There are no config files produced by this build, it is common to have your own FindClipper.cmake to find this library, so also the vcpkg_fixup_cmake_targets() is not necessary

sry, I delete that sentence seconds after I posted it.

may I ask you why should I patch SET(CMAKE_BUILD_TYPE "Release" CACHE STRING "Release type")?

because it overwrites the build type or does it not? Argh CMakes (stupid) syntax rules. set(bla CACHE) only sets the value if it has not been supplied. Sry for that, was a bit too fast.

own FindClipper.cmake

I find that a bit strange because it installs a package configuration file. Although it has not a lot of content.

The port should probably be renamed to polyclipping because there are a lot of other packages called clipper and the first one on google with that name is actually not this port but this: https://github.com/ucbrise/clipper . Furthermore the project name in the CMakelists is polyclipping

may I tell you one thing? You are usually too much fast at writing before documenting yourself. Also in the other PR about LibLZMA, which I am not linking because it's not a good practice to link PR that are not related (something that you do too much, also, spamming references without any reason), there are many broken statements by you, which I just avoided replying to reduce flame... please take your time and check things before spreading noise... otherwise again it's just more work for everybody, as I already told you.
Rant over

About config, there is a pkg-config file produced by CMake, not a native config, so usually it's not even considered during library consumption.
About the static library, since the dynamic library on linux are not even supported and on windows there's not explicit support (so no symbols are available in the library if not statically built), considering that WINDOWS_EXPORT_ALL_SYMBOLS is not accepted, that's the only clean solution available.

Finally, about renaming, Clipper is how it is called and referenced. I'd not rename it if not requested by admins. In my google search page, which I am sure you know can be different from yours, this library is the first result, so that's not a valid metric.

@Neumann-A
Copy link
Contributor

dynamic library on linux are not even supported

that is not 100% correct. There is no triplet supplied by vcpkg but users are free to change the linkage in the triplet or supply their own. We should also consider these users

considering that WINDOWS_EXPORT_ALL_SYMBOLS is not accepted,

there are already a few merged PRs by reverting that change. (A quick search through my port folders reveals 19 hits with only 1 removal of (CMAKE_)WINDOWS_EXPORT_ALL_SYMBOLS). I don't know if it is a good idea just an observation of mine. I probably would only force the static linkage on WIN32 and let somebody else figure out if WINDOWS_EXPORT_ALL_SYMBOLS is appropriate.

Finally, about renaming, Clipper is how it is called and referenced.

In hunter it is also referenced as polyclipping https://docs.hunter.sh/en/latest/packages/pkg/polyclipping.html?highlight=polyclipping
Just for reference so that somebody can decide how the name should be

there are many broken statements by you, which I just avoided replying to reduce flame... please take your time and check things before spreading noise.

If I am wrong just correct me. I don't take it personally or as flame. Nobody knows everything and everybody can and will be wrong at some point. Correcting somebody is the faster experience because other might also wonder about that point and follow it. So please correct wrong arguments/assumptions.

@cenit
Copy link
Contributor Author

cenit commented May 27, 2019

supported here means that there's a triplet supplied by the official master...
Everything that's not there can break at every time, so I'd not like to support something I've never tested. Better to open options later than risking now, IMHO

About the remaining WINDOWS_EXPORT_ALL_SYMBOLS, I know them and together with @vicroms we decided to close that PR without touching them. It was already huge. Being the author of the two main commits in it I remember the anxiety I had, I can only imagine Victor's one. Fixing some small remaining one is a much lighter task and we left if for the expected subsequent PRs (only one in reality is requiring the re-addition of it, AFAIK, due to a big oversight I made)

For the name, I'm open to discussion and change. But still, I know this library as clipper, I know I am not the only one so it would not be so strange for me if it was this one occupying this slot...

@ras0219-msft
Copy link
Contributor

The port should probably be renamed to polyclipping because there are a lot of other packages called clipper and the first one on google with that name is actually not this port but this: https://github.com/ucbrise/clipper . Furthermore the project name in the CMakelists is polyclipping

Based on https://repology.org/project/clipper/versions and https://repology.org/project/polyclipping/versions, it does look like polyclipping is the much more popular term of art for this library. Therefore I'd agree with @Neumann-A -- could you please rename this port to polyclipping?

The pkgconfig/cmake-config files (if any) don't need to be renamed.

@ras0219-msft ras0219-msft self-assigned this May 29, 2019
@cenit
Copy link
Contributor Author

cenit commented May 30, 2019

ok I did it

@cenit cenit changed the title [clipper] new port [polyclipping] new port May 30, 2019
@ras0219-msft ras0219-msft merged commit ba259ef into microsoft:master Jun 3, 2019
@cenit cenit deleted the dev/cenit/clipper branch June 3, 2019 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants