-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[inih] Build with Meson + update to r57 #33001
Conversation
@microsoft-github-policy-service agree |
Compilers differ on what the default standard is and I had to set C++11 standard as part of the [vcpkg port](microsoft/vcpkg#33001) to get it work on MacOS. Also you forgot to bump the version number last release.
Yay! That actually worked 😄
Well meson does have a CMake module that can write a version file, but beyond that it's not super useful |
Compilers differ on what the default standard is and I had to set C++11 standard as part of the [vcpkg port](microsoft/vcpkg#33001) to get it work on MacOS. Also you forgot to bump the version number last release.
@DownerCase, Thanks for your PR, please restore the newline type for portfile.cmake and CMakeLists.txt. |
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags. |
Please run command |
IMO the port should get rid of |
I mean sure, I considered that at one point but it felt wrong somehow to do all that in the portfile so I used the CMakeList instead. The version file was thrown in just to “complete” the package and because it was easy. I’m not attached to it. If you have particular suggestions on how to simplify the cmake.in file I’m all ears. The annoying bit for me was that the meson build generates two library files, as opposed to the current port build which bundles them into one, requiring two CMake imported targets and all the duplication thereof. |
Tesed usage successfully by
|
I'll address the review comments later today. |
- Removed CMake package versioning - Wasn't that useful as upstream don't use semver - Use straight configure_file to install CMake package config - Make cpp feature default - Prefer PkgConfig usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU the package offers two things: a C library, and a C++ class. Even if the latter uses the former, users will only access one or another directly.
IMO usage
should list these patterns separately, e.g.
inih can be imported via CMake FindPkgConfig module:
find_package(PkgConfig)
# C API
pkg_check_modules(inih REQUIRED IMPORTED_TARGET inih)
target_link_libraries(main PRIVATE PkgConfig::inih)
# C++ API
pkg_check_modules(inih REQUIRED IMPORTED_TARGET INIReader)
target_link_libraries(main PRIVATE PkgConfig::INIReader)
Simlilar for the unofficial cmake config.
@dg0yt Your understanding is correct. I'll make the change but this usage is becoming rather long. Is it ok for the C++ API usage to still be present even if the feature has been turned off and it is not present? |
I'm aware of the length. That's why I'm against promoting unofficial usage when there is an official pattern.
I think I made the point before: Remove the optional feature and always install the C++ API. I see no benefit from making it optional in this package. |
You did. I think I misunderstood and made it a default-option. IMO what we have now is good enough; inih is a C library first and they build INIReader as an optional feature, makes sense to me to reflect that, the work's already done for it. Something to revisit next update perhaps. |
Great ci flaked out, 503 when downloading vcpkg tool. Rerunning x64_windows_static_md is needed. |
Tested usage successfully by inih:x64-windows:
|
ports/inih/usage
Outdated
The package inih can be imported via CMake FindPkgConfig module: | ||
|
||
find_package(PkgConfig) | ||
|
||
# C API | ||
pkg_check_modules(INIH REQUIRED IMPORTED_TARGET inih) | ||
target_link_libraries(main PRIVATE PkgConfig::INIH) | ||
|
||
# C++ API (Requires "cpp" feature) | ||
pkg_check_modules(INIReader REQUIRED IMPORTED_TARGET INIReader) | ||
target_link_libraries(main PRIVATE PkgConfig::INIReader) | ||
|
||
Alternatively, the package inih provides unofficial CMake targets: | ||
|
||
find_package(unofficial-inih CONFIG REQUIRED) | ||
|
||
# C API | ||
target_link_libraries(main PRIVATE unofficial::inih::libinih) | ||
|
||
# C++ API (Requires "cpp" feature) | ||
target_link_libraries(main PRIVATE unofficial::inih::inireader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be giving instructions for using pckgconfig through cmake. I think it should just say something like, "if you are using CMake, the package provides unofficial CMake targets ...". If you are using pckgconfig here's the name.
The package inih can be imported via CMake FindPkgConfig module: | |
find_package(PkgConfig) | |
# C API | |
pkg_check_modules(INIH REQUIRED IMPORTED_TARGET inih) | |
target_link_libraries(main PRIVATE PkgConfig::INIH) | |
# C++ API (Requires "cpp" feature) | |
pkg_check_modules(INIReader REQUIRED IMPORTED_TARGET INIReader) | |
target_link_libraries(main PRIVATE PkgConfig::INIReader) | |
Alternatively, the package inih provides unofficial CMake targets: | |
find_package(unofficial-inih CONFIG REQUIRED) | |
# C API | |
target_link_libraries(main PRIVATE unofficial::inih::libinih) | |
# C++ API (Requires "cpp" feature) | |
target_link_libraries(main PRIVATE unofficial::inih::inireader) | |
The package inih provides unofficial CMake targets: | |
find_package(unofficial-inih CONFIG REQUIRED) | |
# C API | |
target_link_libraries(main PRIVATE unofficial::inih::libinih) | |
# C++ API (Requires "cpp" feature) | |
target_link_libraries(main PRIVATE unofficial::inih::inireader) | |
Alternatively, if you are using pckgconfig use the name "inih" for the C API and "inireader" for the C++ API | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve just been going with established patterns, sounds like you guys need to form a consensus and guidance on how to handle usage texts and apply that uniformly. Something not in the scope of this PR which was to update a small package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dg0yt As the initial reviewer would you like to weigh in on this? You expressed a preference for the pkgconfig method previously as it is the officially supported method which I agree with and is why I put the unofficial CMake target second as the alternative usage.
Having full instructions for official usage makes sense to me and is what I would expect to see.
From my perspective as a user there are two things that matter.
- Does the package build cleanly for my case. CI suggests yes.
- Can I read the usage text and get it working in my project. JonLiu has verified that it can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You expressed a preference for the pkgconfig method previously as it is the officially supported method which I agree with and is why I put the unofficial CMake target second as the alternative usage.
Give the feedback from vcpkg's @JavierMatosD, I don't want to ask for more changes. But I do have a clear <opinion>
: Don't add or announce unofficial CMake config (vcpkg lock-in) when there is official pkg-config.
I could accept the short form of announcing the pkgconfig option, but IMO it is the primary form (official, no lock-in), not the alternative.</opinion>
. From that POV, I can't approve, but it doesn't matter anyways.
Can I read the usage text and get it working in my project. JonLiu has verified that it can.
The usual quick validation of usage just scratches the surface. It detects some logical bugs, and this is a good thing. But normally it can't detect semantical defects in usage hints which are taken literally from the uninformed heuristics. So when adding usage
, better spend an extra loop on reviewing it from the perspective of an uninitiated user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fine. @JavierMatosD how about this. We drop the unofficial CMake support completely and only retain the first half of the current usage text? Pkgconfig is made available by upstream and users must use that to use this package.
Downstream would just have to deal with the unofficial target disappearing on them. I know this at least this will affect minio-cpp. If this goes forward I will sort out the change with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unofficial CMake support has been dropped in favor of pkgconfig. If people have trouble migrating, tag me in and I'll help.
I tried to do some work on my Exiv2 update after dropping it and found out why it should stick around. Becomes required if something is using it and tries to declare their own CMake targets to link to. They stop being available once you install.
I've taken your usage change suggestion. Hopefully that's all now 🙏
This reverts commit 94ff6a9.
My respect for all the vcpkg maintainers has grown hugely after getting a taste of the madness you have to deal with. Thank you for all the hard work to make this all usable. Anyway, hopefully this PR is almost done now... |
[inih] Build with Meson + update to r57 (microsoft#33001)
Follows on from #17388
Fixes #17385
Done in preparation to continue #31722 and properly address this review comment
If this PR updates an existing port, please uncomment and fill out this checklist:
./vcpkg x-add-version --all
and committing the result.Repo I've used to verify proper usage: https://github.com/DownerCase/vcpkg_verifier/tree/inih
Writing a CMake package for this by hand was an interesting experience. There must be a better way to do this because it won't work for anything more complex.