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

[reflectcpp] Update to 0.17.0 #43135

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

liuzicheng1987
Copy link
Contributor

@liuzicheng1987 liuzicheng1987 commented Jan 6, 2025

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@liuzicheng1987
Copy link
Contributor Author

reflectcpp 0.17.0 adds Cap'n Proto, so I have added this as a feature.

When I submitted version 0.16.0, there was some discussion on the way I was linking CBOR and TOML. Back then, I decided to just leave them out for now, fix the issues you had raised and then include them with the next release. I have done just that, so now CBOR and TOML are included as features as well.

Moreover, I have improved the package description.

@JonLiu1993 JonLiu1993 added category:port-update The issue is with a library, which is requesting update new revision category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jan 7, 2025
@JonLiu1993
Copy link
Member

@liuzicheng1987, Thanks for your PR, when I tested the features by command ./vcpkg install reflectcpp[*]:x64-windows, I get this error, please take a look:

CMake Error at F:/Feature-test/reflectcpp/downloads/tools/cmake-3.30.1-windows/cmake-3.30.1-windows-i386/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
Call Stack (most recent call first):
  F:/Feature-test/reflectcpp/downloads/tools/cmake-3.30.1-windows/cmake-3.30.1-windows-i386/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
  F:/Feature-test/reflectcpp/downloads/tools/cmake-3.30.1-windows/cmake-3.30.1-windows-i386/share/cmake-3.30/Modules/FindPkgConfig.cmake:114 (find_package_handle_standard_args)
  F:/Feature-test/reflectcpp/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
  CMakeLists.txt:187 (find_package)

@liuzicheng1987
Copy link
Contributor Author

Hi @JonLiu1993 ,

that's a problem with the TOML port. It doesn't provide a cmake.config, just a PkgConfig, so you need PkgConfig to link the TOML port.

This was in fact the solution that was suggested by the vcpkg reviewers the last time, otherwise we cannot support TOML.

@liuzicheng1987
Copy link
Contributor Author

@JonLiu1993, let me point you to PR #42461.

You will see that I have done exactly what the reviewers suggested the last time and also what the usage file in the TOML package suggests.

If you are not happy with that, then we won't be able to support TOML until someone adds a cmake.config to the TOML port.

@liuzicheng1987
Copy link
Contributor Author

@JonLiu1993, here is the usage file of the TOML port:

https://github.com/microsoft/vcpkg/blob/0f0fa15383f4ef0437940b1be287d4e217400db2/ports/tomlplusplus/usage

I have integrated these lines into my own CMakeLists.txt upstream.

We can either live with the fact that you would need PkgConfig or I could just take out the TOML feature again.

I would personally prefer leaving it in...I think the error message you have posted is pretty clear.

But, of course, you're the reviewer, so it's your call.

@JonLiu1993
Copy link
Member

If I install pkgconf port first, all features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-static

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jan 7, 2025
@liuzicheng1987
Copy link
Contributor Author

@JonLiu1993, thanks.

@JavierMatosD
Copy link
Contributor

We can either live with the fact that you would need PkgConfig or I could just take out the TOML feature again.

Just skimming through this but I think you should be able to add vcpkg_find_acquire_program(PKGCONFIG) to the portfile.cmake and then pass "-DPKG_CONFIG_EXECUTABLE=${PKGCONFIG}" option to vcpkg_cmake_configure

I was able to build just fine after adding that.

@JavierMatosD JavierMatosD marked this pull request as draft January 8, 2025 17:30
@liuzicheng1987
Copy link
Contributor Author

@JavierMatosD , wouldn't I have to make upstream changes for that?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 8, 2025

Just skimming through this but I think you should be able to add vcpkg_find_acquire_program(PKGCONFIG) to the portfile.cmake and then pass "-DPKG_CONFIG_EXECUTABLE=${PKGCONFIG}" option to vcpkg_cmake_configure

@JavierMatosD This doesn't help. There is no problem building the port. The problem is using it. Possible mitigations:

  • Make the package build use <PREFIX>_LINK_LIBRARIES. This removes the need to deal with pkg-config at usage time.
  • Make the port depend on host pkgconf and ensure that it is found when the port is used - i.e. the CMake package or a wrapper would need to inject the path to the host pkgconf.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 8, 2025

Actually there is also the problem with building the port. It can be resolved by the proposal by @JavierMatosD. This part doesn't need upstream changes.

@liuzicheng1987
Copy link
Contributor Author

liuzicheng1987 commented Jan 8, 2025

@JavierMatosD @dg0yt, how much effort would it be to just add a cmake.config to the TOML port?

Just to be clear: This is only a problem if you want TOML support...and if the rule is that you don't want people to have to use PkgConfig, wouldn't it be the cleanest solution to just fix the root cause of the problem?

If it isn't too much effort, I would be happy to write a patch for tomlplusplus...

@dg0yt
Copy link
Contributor

dg0yt commented Jan 8, 2025

It might be possible and desirable to use tomlplusplus' CMake build system. But IIUC you would have to add back pkgconfig support.
marzer/tomlplusplus#209

@liuzicheng1987
Copy link
Contributor Author

liuzicheng1987 commented Jan 8, 2025

@JonLiu1993 @JavierMatosD @dg0yt , since I don't think the problem can be fully resolved without upstream changes, I have decided to once again remove TOML support. This removes the need for PkgConfig and thus avoids the problem altogether.

I will take a look at what I can do to fix the problem upstream, either by writing some kind of workaround in reflect-cpp or submitting a PR to tomlplusplus.

@JonLiu1993 JonLiu1993 removed the info:reviewed Pull Request changes follow basic guidelines label Jan 9, 2025
@JonLiu1993
Copy link
Member

If this PR is ready for review, please let me know, thanks.

@liuzicheng1987 liuzicheng1987 marked this pull request as ready for review January 9, 2025 07:48
@liuzicheng1987
Copy link
Contributor Author

liuzicheng1987 commented Jan 9, 2025

@JonLiu1993 , sorry, my mistake. I forgot to hit the "Ready for Review" button.

I have resolved the PkgConfig issue by simply removing the TOML feature once again. I will find a solution by either submitting a PR to tomlplusplus or by simply using something like toml11 instead. But I won't do that until the next release of reflect-cpp.

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed info:reviewed Pull Request changes follow basic guidelines labels Jan 9, 2025
@JonLiu1993
Copy link
Member

when I tested the features by command ./vcpkg install reflectcpp[*]:x64-linux, I get this error, please take a look:


In file included from /home/test/Jon/vcpkg/buildtrees/reflectcpp/src/v0.17.0-9603e9c4ca.clean/include/rfl/Field.hpp:10,
                 from /home/test/Jon/vcpkg/buildtrees/reflectcpp/src/v0.17.0-9603e9c4ca.clean/include/rfl/AddStructName.hpp:6,
                 from /home/test/Jon/vcpkg/buildtrees/reflectcpp/src/v0.17.0-9603e9c4ca.clean/include/rfl.hpp:10,
                 from /home/test/Jon/vcpkg/buildtrees/reflectcpp/x64-linux-dbg/CMakeFiles/reflectcpp.dir/cmake_pch.hxx:5,
                 from <command-line>:
/home/test/Jon/vcpkg/buildtrees/reflectcpp/src/v0.17.0-9603e9c4ca.clean/include/rfl/Literal.hpp:4:10: fatal error: compare: No such file or directory
    4 | #include <compare>
      |          ^~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.

@liuzicheng1987
Copy link
Contributor Author

liuzicheng1987 commented Jan 9, 2025

@JonLiu1993 , this must be an issue with your compiler. What compiler and what version are you using here?

<compare> is a C++-20 standard library header.

https://en.cppreference.com/w/cpp/header/compare

@liuzicheng1987
Copy link
Contributor Author

liuzicheng1987 commented Jan 9, 2025

Even though this may be the most hackneyed thing for a developer to say: It works perfectly fine on my Linux machine.

(And I am also very sure that the automated tests wouldn't have passed if there were an actual issue.)

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jan 10, 2025
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 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.

4 participants