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

[kdreports] new port #42625

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[kdreports] new port #42625

wants to merge 1 commit into from

Conversation

drdanz
Copy link
Contributor

@drdanz drdanz commented Dec 10, 2024

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 11, 2024
ports/kdreports/portfile.cmake Outdated Show resolved Hide resolved
@jimwang118
Copy link
Contributor

I got the following error when testing usage locally.

Severity	Code	Description	Project	File	Line	Suppression State	Details
Error		CMake Error at F:/kdp/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
  By not providing "FindQtCore.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "QtCore", but
  CMake did not find one.

  Could not find a package configuration file provided by "QtCore" (requested
  version 6.0.0) with any of the following names:

    QtCoreConfig.cmake
    qtcore-config.cmake

  Add the installation prefix of "QtCore" to CMAKE_PREFIX_PATH or set
  "QtCore_DIR" to a directory containing one of the above files.  If "QtCore"
  provides a separate development package or SDK, be sure it has been
  installed.		F:/kdp/scripts/buildsystems/vcpkg.cmake	859		

@jimwang118 jimwang118 marked this pull request as draft December 11, 2024 06:10
@drdanz
Copy link
Contributor Author

drdanz commented Dec 11, 2024

  Could not find a package configuration file provided by "QtCore" (requested
  version 6.0.0) with any of the following names:

    QtCoreConfig.cmake
    qtcore-config.cmake

That's weird, QtCoreConfig.cmake should be in the qtbase package, which is an explicit dependency, and it is normally found automatically (I don't see any other port doing anything strange to find Qt6). I tried adding an explicit dependency on the widget component, perhaps that was the problem since I set "default-features": false. @jimwang118 Could you please try again?

@drdanz drdanz requested a review from jimwang118 December 11, 2024 13:08
@drdanz drdanz marked this pull request as ready for review December 11, 2024 13:42
@jimwang118
Copy link
Contributor

That's weird, QtCoreConfig.cmake should be in the qtbase package, which is an explicit dependency, and it is normally found automatically (I don't see any other port doing anything strange to find Qt6). I tried adding an explicit dependency on the widget component, perhaps that was the problem since I set "default-features": false. @jimwang118 Could you please try again?

Still reporting the same error.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 12, 2024

The documented CMake usage for Qt6 is:

find_package(Qt6 REQUIRED COMPONENTS Core)

https://doc.qt.io/qt-6/cmake-get-started.html

This is what is supported in vcpkg.

@drdanz
Copy link
Contributor Author

drdanz commented Dec 12, 2024

The documented CMake usage for Qt6 is:

find_package(Qt6 REQUIRED COMPONENTS Core)

https://doc.qt.io/qt-6/cmake-get-started.html

This is what is supported in vcpkg.

This is what the package is already doing, see https://github.com/KDAB/KDReports/blob/master/CMakeLists.txt#L118
It looks more like a problem in Qt6 port than in this... @jimwang118 Perhaps you are building on some untested platform/triplet? CI is passing on all tested platforms... Can you tell me what architecture are you building on, so that I can try reproducing it? Thanks

@jimwang118
Copy link
Contributor

I just created a cmake project on VS, and then called usage to compile it, and the above problem was reported.

@iamsergio
Copy link

I just created a cmake project on VS, and then called usage to compile it, and the above problem was reported.

Does kdsoap work for you ? It's an existing port which is similar to kdreports

@jimwang118
Copy link
Contributor

Does kdsoap work for you ? It's an existing port which is similar to kdreports

The kdsoap test is normal and can be used.

@drdanz
Copy link
Contributor Author

drdanz commented Dec 12, 2024

Found the bug, it should be fixed now, I'll fix it upstream as well


Edit: bug is already fixed in latest master

@dg0yt
Copy link
Contributor

dg0yt commented Dec 12, 2024

You found the typo in the right file now.

I still think that dependency lookup up in the project and in the exported config should follow the same pattern, i.e.

find_package(Qt6 ${QT_MIN_VERSION} CONFIG REQUIRED COMPONENTS Core Widgets PrintSupport Xml)

should be paired with

find_dependency(Qt6 @QT_MIN_VERSION@ CONFIG COMPONENTS Core Widgets PrintSupport Xml)

which maps to exactly the same config lookup and targets, instead of requesting a different set of packages.
(Note that find_dependency is not to be used with REQUIRED.)

@jimwang118
Copy link
Contributor

Usage test passed with x64-windows triplet.

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants