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

[qtbase] qdbuscpp2xml subtly broken on Windows by recent port update #39040

Closed
tsondergaard opened this issue May 30, 2024 · 10 comments · Fixed by #38845
Closed

[qtbase] qdbuscpp2xml subtly broken on Windows by recent port update #39040

tsondergaard opened this issue May 30, 2024 · 10 comments · Fixed by #38845
Assignees
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist

Comments

@tsondergaard
Copy link
Contributor

qdbuscpp2xml is a tool from qtbase that "Parses the C++ source or header file containing a QObject-derived class and produces the D-Bus Introspection XML." This tool no longer works on Windows because it cannot load dbus-1-3.dll. I can see that in my project, prior to the latest vcpkg baseline update I have dbus-1-3.dll in vcpkg_installed\x64-windows\tools\Qt6\bin and after the baseline update it is no longer there. The changes in files listed in that directory is this:

6,7d5
< assistant.exe
< assistant.pdb
13d10
< dbus-1-3.dll
62,63d58
< qhelpgenerator.exe
< qhelpgenerator.pdb
132,133d126
< Qt6Help.dll
< Qt6Help.pdb
275a269,270
> svgtoqml.exe
> svgtoqml.pdb

The removal of assistant and Qt6Help is because of #38612. The addition of svgtoqml is due to #38682. I think the removal of dbus-1-3.dll is also due to #38682 because of https://github.com/microsoft/vcpkg/pull/38682/files#diff-da814fe5d578d93392e698157af490aeb2f7994eecdf94b272356ae97661ddc4L286

qdbuscpp2xml is only subtly broken as it only conditionally load libdbus-1-3.dll. It will load it if the C++ class uses custom types like QVariantMap. The code in question is https://github.com/qt/qtbase/blob/dev/src/dbus/qdbusargument.cpp#L36. If the dbus library fails to load the generated xml will have type="" instead of type="a{sv}" as it should.

Environment

  • OS: Windows
  • Compiler: Visual C++ 2022

To Reproduce
Steps to reproduce the behavior:

  1. ./vcpkg install qtbase
  2. Set environment variable QT_LOGGING_RULES=qt.core.library.debug=true
  3. Run vcpkg_installed\<triplet>\tools\Qt6\bin\qdbuscpp2xml.exe example.h

Expected behavior
The generated xml output should contain <arg type="a{sv}" direction="out"/> and no debug messages should be logged

Actual behavior
The generated xml output contains <arg type="" direction="out"/> (type is empty) and the following messages are logged by Qt:

qt.core.library: "dbus-1-3" cannot load: Cannot load library dbus-1-3: The specified module could not be found.
qt.core.library: "libdbus-1-3" cannot load: Cannot load library libdbus-1-3: The specified module could not be found.
qt.core.library: "dbus-1-2" cannot load: Cannot load library dbus-1-2: The specified module could not be found.
qt.core.library: "libdbus-1-2" cannot load: Cannot load library libdbus-1-2: The specified module could not be found.
qt.core.library: "dbus-1" cannot load: Cannot load library dbus-1: The specified module could not be found.
qt.core.library: "libdbus-1" cannot load: Cannot load library libdbus-1: The specified module could not be found.

example.h for the repro:

#ifndef EXAMPLE_H
#define EXAMPLE_H
#include <QObject>
#include <QVariantMap>

class MyInterface : public QObject
{
    Q_OBJECT
    Q_CLASSINFO("D-Bus Interface", "foo.MyInterface");
public Q_SLOTS:
    int getInt() const;
    QVariantMap getVariantMap() const;
};

#endif
@tsondergaard
Copy link
Contributor Author

tsondergaard commented May 30, 2024

@Neumann-A, don't you think this is caused by https://github.com/microsoft/vcpkg/pull/38682/files#diff-da814fe5d578d93392e698157af490aeb2f7994eecdf94b272356ae97661ddc4L286? How do you think it should be fixed?

For projects that want to use dbus on Windows, and we are probably very few that do, wouldn't it make sense to be able to use dbus=linked? That should be sufficient to pull the dbus-1-3.dll library into the vcpkg_installed\x64-windows\tools\Qt6\bin, should it not?

@tsondergaard
Copy link
Contributor Author

tsondergaard commented May 30, 2024

As an experiment I applied the following patch and that does indeed solve the problem:

diff --git a/ports/qtbase/portfile.cmake b/ports/qtbase/portfile.cmake
index d23ed5fee6..0e77740424 100644
--- a/ports/qtbase/portfile.cmake
+++ b/ports/qtbase/portfile.cmake
@@ -95,10 +95,8 @@ INVERTED_FEATURES
 list(APPEND FEATURE_OPTIONS -DCMAKE_DISABLE_FIND_PACKAGE_Libudev:BOOL=ON)
 list(APPEND FEATURE_OPTIONS -DFEATURE_xml:BOOL=ON)

-if("dbus" IN_LIST FEATURES AND VCPKG_TARGET_IS_LINUX)
+if("dbus" IN_LIST FEATURES)
   list(APPEND FEATURE_OPTIONS -DINPUT_dbus=linked)
-elseif("dbus" IN_LIST FEATURES)
-  list(APPEND FEATURE_OPTIONS -DINPUT_dbus=runtime)
 else()
   list(APPEND FEATURE_OPTIONS -DINPUT_dbus=no)
 endif()
diff --git a/ports/qtbase/vcpkg.json b/ports/qtbase/vcpkg.json
index 87020883b3..2b7c640a2c 100644
--- a/ports/qtbase/vcpkg.json
+++ b/ports/qtbase/vcpkg.json
@@ -126,8 +126,7 @@
       "dependencies": [
         {
           "name": "dbus",
-          "default-features": false,
-          "platform": "linux"
+          "default-features": false
         },
         {
           "name": "qtbase",

I understand this may not be an acceptable solution, I am just providing this information as confirmation for what works.

@MonicaLiu0311 MonicaLiu0311 added the requires:repro The issue is not currently repro-able label May 30, 2024
@Neumann-A
Copy link
Contributor

three Questions:
a) what is qt's default? (since that is what people expect)
b) I feel like the dbus dll could simply be installed by qtbase into the correct folder (there are already a few other dlls which get manually moved)
c) could this be fixed by adjusting qt.conf somehow or is the dll lookup not using qt.conf paths?

and there should definitely be a test port for this use case.

@tsondergaard
Copy link
Contributor Author

three Questions: a) what is qt's default? (since that is what people expect)

Default is dbus=runtime on Windows, I believe.

b) I feel like the dbus dll could simply be installed by qtbase into the correct folder (there are already a few other dlls which get manually moved)

That works for me. What are the existing examples?

c) could this be fixed by adjusting qt.conf somehow or is the dll lookup not using qt.conf paths?

Looks like no. Grepping the Qt source code for QLibraryInfo::LibrariesPath it is not used in the QLibrary code and the documentation also says it looks in system-defined places with no mention of QLibraryInfo.

and there should definitely be a test port for this use case.

I'm not familiar with test ports, but I can see there is a directory vcpkg.git/scripts/test_ports. If I exclude all the subdirectories that start with vcpkg- that directory is very lightly populated. Am I looking at the right thing?

@Neumann-A
Copy link
Contributor

Am I looking at the right thing?

Yes. test_ports are ports build in CI to test usage etc.

system-defined places with no mention of QLibraryInfo.

Probably means PATH then.

That works for me. What are the existing examples?

set(qtbase_owned_dlls
double-conversion.dll
icudt74.dll
icuin74.dll
icuuc74.dll
libcrypto-3-${VCPKG_TARGET_ARCHITECTURE}.dll
libcrypto-3.dll # for x86
pcre2-16.dll
zlib1.dll
zstd.dll
)

@tsondergaard
Copy link
Contributor Author

@Neumann-A, seems straight-forward to add zlib-1-3.dll to that list as it already checks if the files exist before trying to copy them.

Regard the test_ports it looks like most of them just consist of a vcpkg.conf that enable certain features and a portfile with set(VCPKG_POLICY_EMPTY_PACKAGE enabled). The test for this would be a little more elaborate as we would either have to run qdbuscpp2xml to test directly or just check for the presence of dbus-1-3.dll in the vcpkg_installed\x64-windows\tools\Qt6\bin folder. What are your thoughts on this?

@Neumann-A
Copy link
Contributor

as we would either have to run qdbuscpp2xml to test directly

That was the idea. I mean you already gave an example.

@dg0yt
Copy link
Contributor

dg0yt commented May 30, 2024

Basically, all DLLs needed by a Qt plugin (DBus) needed by Qt tools (qdbuscpp2xml) must be deployed to the tools dir. By the port which installs the plugin.

@tsondergaard
Copy link
Contributor Author

tsondergaard commented May 30, 2024

I tried this simple patch:

diff --git a/ports/qtbase/portfile.cmake b/ports/qtbase/portfile.cmake
index d23ed5fee6..06f15433bc 100644
--- a/ports/qtbase/portfile.cmake
+++ b/ports/qtbase/portfile.cmake
@@ -513,6 +513,7 @@ if(VCPKG_TARGET_IS_WINDOWS)
   # dlls owned but not automatically installed by qtbase
   # this is required to avoid ownership troubles in downstream qt modules
   set(qtbase_owned_dlls
+        dbus-1-3.dll
         double-conversion.dll
         icudt74.dll
         icuin74.dll

and discovered something ugly that I had not realized - the files become part of the built qtbase package. This is nasty for dbus-1-3.dll as the qtbase port doesn't have a dependency on the dbus port so the dbus-1-3.dll will be included and stored in the package in the binary cache if it was present when qtbase was built. In other words the same package hash is used regardless of whether dbus-1-3.dll is included or not.

I believe the consequence is that dbus should be a formal dependency when the dbus feature is enabled. If we do that we may as well use dbus=linked always. Since dbus is very infrequently used on the Windows platform it would make sense to remove dbus from the default feature list on Windows. What do you think?

Is it possible to have different default features on default platforms? Is it a bad idea?

@Neumann-A
Copy link
Contributor

I'll vote for keeping it runtime since that is what upstream Qt shipps. However, I would remove it as a default feature on windows and add the dep to the feature without a platform expressions.

@MonicaLiu0311 MonicaLiu0311 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist and removed requires:repro The issue is not currently repro-able labels Jun 6, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants