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

[readline-(osx|win32|unix)] remove platform dependent ports. #22436

Closed
Neumann-A opened this issue Jan 8, 2022 · 10 comments
Closed

[readline-(osx|win32|unix)] remove platform dependent ports. #22436

Neumann-A opened this issue Jan 8, 2022 · 10 comments
Assignees
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist Stale

Comments

@Neumann-A
Copy link
Contributor

there should only be one readline port. (see how openssl does it.....)

@Neumann-A Neumann-A changed the title [readline] remove platform dependent ports. [readline-(osx|win32|unix)] remove platform dependent ports. Jan 8, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Jan 9, 2022

Only 27 days ago: #22000 (comment)

@JackBoosY
Copy link
Contributor

@dg0yt Now readline uses makefile in non-Windows.

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jan 10, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Jan 10, 2022

@dg0yt Now readline uses makefile in non-Windows.

I know. Still, it would have been less work to just implement it in port readline, right from my first comment, before adding new names to the catalog.
And it seems we need a vcpkg-pkgconfig-validate as much as the submitted vcpkg-cmake-validate.

@Neumann-A
Copy link
Contributor Author

vcpkg-pkgconfig-validate

In the past vcpkg_fixup_pkgconfig had validation but it was removed since a lot of pc files contain system libs in Libs:. and vcpkg cannot guess all those for all cases.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 10, 2022

In the past vcpkg_fixup_pkgconfig had validation but it was removed since a lot of pc files contain system libs in Libs:. and vcpkg cannot guess all those for all cases.

No guessing needed. Builds a test project which consumes the pc file. It must break when there is -lm for x64-windows.

@Neumann-A
Copy link
Contributor Author

No guessing needed.

#13126 was the pr removing the library checks.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 11, 2022

#13126 was the pr removing the library checks.

However, these were pure script mode checks. I mean building a test project. Takes more CI build time, but should catch such errors quite reliably.

@Neumann-A
Copy link
Contributor Author

I mean building a test project.

requires symbol usage to detect stuff. And even that is not reliable. If you have already ports depending on the upstream pkgconfig stuff that is better than any arbitrary test.
In generally the only test needed is to see if the stuff in Libs: is existing.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 11, 2022

If you have already ports depending on the upstream pkgconfig stuff that is better than any arbitrary test.

The problem is that we sometimes/often don't have the ports (yet), or the feature combination isn't tested (yet), or the dependency is not used (yet) due to some cascaded skip, or whatever. When a problem appears in some consuming configuration, it is hard to track down the origin of the problem.

Copy link

This is an automated message. Per our repo policy, stale issues get closed if there has been no activity in the past 180 days. The issue will be automatically closed in 14 days. If you wish to keep this issue open, please add a new comment.

@github-actions github-actions bot added the Stale label Jun 29, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants