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

[turf, junction] new ports #17540

Closed
wants to merge 15 commits into from
Closed

[turf, junction] new ports #17540

wants to merge 15 commits into from

Conversation

mathisloge
Copy link
Contributor

Describe the pull request
adds turf and junction

yes

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

    yes

@mathisloge mathisloge changed the title Junction [turf, junction] new ports Apr 27, 2021
@mathisloge
Copy link
Contributor Author

@xanderdunn please review and if you have time, please test

@mathisloge mathisloge marked this pull request as draft April 27, 2021 16:52
@mathisloge mathisloge marked this pull request as ready for review April 27, 2021 16:56
@xanderdunn
Copy link

@mathisloge huge thanks for putting this together. After further development with junction we've decided to move away from it for several reasons:

  • junction only supports raw pointers as keys and values in the hash map. smart pointers are not supported. This puts the burden of memory management on the developer.
  • linking with junction's turf dependency can cause problems when trying to link with -fsanitize=address because both turf and the address sanitizer library define the same symbols.
  • Requires calling an update function from every thread that's accessing the concurrent hash map or memory will leak
    We've replaced our usage with libcuckoo, which is already a part of vcpkg, and so far it appears to solve the above issues

@mathisloge your original hesitation around the junction package was in the right direction.

junction can still be used successfully if a developer is careful, I think it's your call whether to include the new port. I'm happy with closing my original issue either way.

@mathisloge
Copy link
Contributor Author

thanks for the explanations @xanderdunn .

I would move the decision if this should be included in vcpkg to the person who reviews this PR.
But as both seems to not actively developed it might be a good solution to move turf and junction into a registry. I can move them into my https://github.com/mathisloge/vcpkg-registry

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 28, 2021
@mathisloge
Copy link
Contributor Author

@JackBoosY please decide on the comment above. Thanks

ports/turf/export-module.patch Outdated Show resolved Hide resolved
ports/turf/export-module.patch Outdated Show resolved Hide resolved
ports/junction/Config.cmake.in Outdated Show resolved Hide resolved
ports/junction/portfile.cmake Show resolved Hide resolved
ports/junction/use-vcpkg-turf.patch Outdated Show resolved Hide resolved
ports/turf/portfile.cmake Show resolved Hide resolved
ports/turf/vcpkg.json Outdated Show resolved Hide resolved
ports/junction/vcpkg.json Outdated Show resolved Hide resolved
@mathisloge
Copy link
Contributor Author

mathisloge commented Apr 28, 2021

@JackBoosY whats your opinion on #17540 (comment) ?

@JackBoosY
Copy link
Contributor

@mathisloge In my opinion, if the port is used by many users, it can be added to vcpkg even if there are some defects, unless the upstream has abandoned it.
vcpkg provides methods to build, install and use ports, but if the port itself contains bugs, we should let the upstream solve them.

@JackBoosY
Copy link
Contributor

Waiting for #17528 merge.

@mathisloge
Copy link
Contributor Author

mathisloge commented Apr 28, 2021

I really don't know if the library is used by many users. @xanderdunn was the first one requesting this lib, but then switched back to another library. turf (last commit 4 years ago) and junction(last commit 3 years ago) are both experimental libraries. And from the comments in some issues from the upstream maintainer it seems like it wouldn't be further developed.

For this reason, I don't know if these two are usable as vcpkg ports. But this decision is up to you. :)
I can also move them over to a external registry.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Apr 28, 2021
@JackBoosY
Copy link
Contributor

turf has almost 500 stars and junction has over 1000 stars, so I think they can be added in vcpkg.

@mathisloge
Copy link
Contributor Author

@JackBoosY merged master after #17528 was merged and now CI is good

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels Apr 29, 2021
@strega-nil-ms
Copy link
Contributor

Marking requires:discussion, as I want to seriously consider whether we wish to include two experimental and unmaintained libraries inside vcpkg; I think that @mathisloge is right, and that registries are likely the correct way forward for these ports.

@strega-nil-ms
Copy link
Contributor

Since there is no upstream interest in maintaining these libraries, the builtin vcpkg registry is not the right place for them. @mathisloge, the perfect place for these is in your custom registry 😄

Thank you for the PR!

@mathisloge
Copy link
Contributor Author

thanks for the feedback @strega-nil-ms have added them to https://github.com/mathisloge/vcpkg-registry

@mathisloge mathisloge deleted the junction branch April 29, 2021 21:38
@mathisloge
Copy link
Contributor Author

maybe #17433 should be closed as well? @strega-nil-ms

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.

[New Port Request] junction
5 participants