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

[poly2tri] Switch to maintained repo and adjust description #26301

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

m-kuhn
Copy link
Contributor

@m-kuhn m-kuhn commented Aug 11, 2022

Describe the pull request

  • What does your PR fix?

    The current repository hasn't seen any commits in 9 years. The new one is actively maintained.
    The description didn't match the repo, the old one was for http://www.angusj.com/delphi/clipper.php

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

    all

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for poly2tri have changed but the version was not updated
version: 2022-08-11
old SHA: 912003223dd4d637d01f6e4d6b9246900b6deebb
new SHA: d71e9f1eb767f59de35a040509a3d85a2ef60033
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/poly2tri/vcpkg.json

Valid values for the license field can be found in the documentation

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Aug 11, 2022

Note to self: this is also vendored in one of the qt5 ports and leads to duplicate symbols if used in combination.

github-actions[bot]
github-actions bot previously approved these changes Aug 11, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/poly2tri/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions[bot]
github-actions bot previously approved these changes Aug 11, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/poly2tri/vcpkg.json

Valid values for the license field can be found in the documentation

@Osyotr
Copy link
Contributor

Osyotr commented Aug 11, 2022

The upstream already has CMakeLists.txt, maybe patch it instead?
Also, it supports dynamic linkage.

github-actions[bot]
github-actions bot previously approved these changes Aug 11, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/poly2tri/vcpkg.json

Valid values for the license field can be found in the documentation

@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Aug 12, 2022
REPO greenm01/poly2tri
REF 88de49021b6d9bef6faa1bc94ceb3fbd85c3c204
SHA512 fa256bcf923ad59f42205edf5a7e07cac6cbd9a37cefb9a0961a2e06aea7fa8ffd09d4e26154c0028601c12804483842cb935d9f602385f5f203c9628382c4fb
REPO jhasse/poly2tri
Copy link
Contributor

@JackBoosY JackBoosY Aug 12, 2022

Choose a reason for hiding this comment

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

Is the official maintains this repo now?
The official link http://code.google.com/p/poly2tri/ is jump to https://github.com/greenm01/poly2tri .

Copy link
Contributor Author

@m-kuhn m-kuhn Aug 12, 2022

Choose a reason for hiding this comment

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

I don't know if "official" applies.
last commit greenm01: on May 2, 2013
last commit jhasse: on May 20, 2022

Copy link
Contributor

Choose a reason for hiding this comment

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

Although jhasse/poly2tri contains commits from contributors to the original repo, this does not prove that this repo is official.
@jhasse is your repo official?

Copy link
Contributor

Choose a reason for hiding this comment

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

I forked it in 2012 to GitHub, because I didn't like Google Code. I've also contacted Mason Green and notified him. His answer:

No problem.. It's Open Source! If you make any significant improvements let me know so I can roll them back into the project.

Have fun.

/Mason

On Thu, 12 Jul 2012 13:39:52 -0400, Jan Niklas Hasse wrote:

Hi!

I just wanted to let you know, that I forked poly2tri GitHub:
https://github.com/jhasse/poly2tri

I hope that's okay?

Greets,
Jan

So nothing official, but if you want I can ask him? I'm still using my fork.

Google Code redirects to his GitHub repo because it was automatically migrated IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is to create a new port and rename it to jhasse-poly2tri to avoid conflict with the original port.
@BillyONeal What do you think about?

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is to create a new port and rename it to jhasse-poly2tri to avoid conflict with the original port. @BillyONeal What do you think about?

I agree it would be better to include the name of the fork in the thing; I'm concerned though that it almost certainly breaks our 'ports can be simultaneously installed' intent. Let me ask some other maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any update on this?
FTR, qtpositioning has a vendored dependency on poly2tri. I'd like to unvendor. If there are 2 different ports that provide this a choice has to be made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any update on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @BillyONeal
Is there any new progress on this matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I couldn't reach Mason Green, he seems to have a new email.

@BillyONeal BillyONeal added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 16, 2022
@JackBoosY JackBoosY changed the title [poly2tri ]Switch to maintained repo and adjust description [poly2tri] Switch to maintained repo and adjust description Aug 17, 2022
@JackBoosY JackBoosY assigned MonicaLiu0311 and unassigned JackBoosY Oct 14, 2022
@BillyONeal
Copy link
Member

@JavierMatosD @vicroms @markle11m @dan-shaw @AugP and I discussed this today.

The competing interests here:

  • We want things in the curated catalog to be maintained.
  • We want names to not change 'ownership' unless the new owner has clearly and completely superseded the old owner.

We can't make both of these work out. We discussed the following:

(A)

  1. Add a new port, jhasse-poly2tri that points at the fork.
  2. Add a new version of poly2tri that has only a dependency on jhasse-poly2tri
  3. Merge (1+2)
  4. Create another PR that removes poly2tri from the baseline.

(B)
We accept that this fork has effectively replaced the original and will merge this PR as is (on this point anyway). If the original becomes maintained again we reserve the right to change it to point back to the original. (Although we will not do so if the fork has effectively eaten the entire userbase of the original more widely)

(C)
We don't accept this at all, please add to a community registry. We will look at bringing something like this back if/when we get alternatives as a first class concept.

We didn't get consensus for any of these, and it doesn't seem like this will change unless the wider world definitively replaces the original with the fork, or we get alternatives.

@JavierMatosD Neutral (A), like (C), don't like (B).
@vicroms Likes (A)
@markle11m Likes (A)
@dan-shaw Likes (A)
@AugP Does not like (C) but accepts the others.
@BillyONeal Does not know.

If jhasse-poly2tri effectively replaces all use of the original we would clearly do (B); repology shows that most folks are still using the original but freebsd has adopted the fork.

@MonicaLiu0311
Copy link
Contributor

Ping @m-kuhn

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Feb 3, 2023

What's expected from me?

@MonicaLiu0311
Copy link
Contributor

What's expected from me?

Although everyone expresses their opinions, the result of the team discussion is more biased towards A. Do you accept the result of A?

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Feb 10, 2023

On what should qtpositioning depend in this scenario?

@BillyONeal
Copy link
Member

@m-kuhn Do you mind if I push changes consistent with option (A) above?

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Jul 26, 2023

@m-kuhn Do you mind if I push changes consistent with option (A) above?

Not at all, please go ahead!

@vicroms vicroms self-assigned this Sep 22, 2023
# Conflicts:
#	ports/poly2tri/CMakeLists.txt
#	ports/poly2tri/vcpkg.json
#	versions/baseline.json
#	versions/p-/poly2tri.json
@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 30, 2024
@BillyONeal
Copy link
Member

@m-kuhn Please confirm that the changes I pushed are agreeable to you.

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Jan 30, 2024

Yes, that looks great, thanks

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Feb 1, 2024
@data-queue data-queue merged commit f7c5a7c into microsoft:master Feb 1, 2024
16 checks passed
TomKatom pushed a commit to TomKatom/vcpkg that referenced this pull request Feb 23, 2024
…t#26301)

* [poly2tri ]Switch to maintained repo and adjust description

* Rename to jhasse-poly2tri

* Also update version and fix header installation.

* Remove double nested headers.

---------

Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants