[qtbase] add namespace support #22713
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
|
This cannot be done as a feature: It changes source compatibility. A consuming port which is not prepared for Qt with namespaces will break when another port requires Qt with namespaces. |
So we just check if QT_NAMESPACE is set by the triplet? The point of the feature is to make it discoverable, but the feature is not part of the defaults and needs QT_NAMESPACE to be set in the triplet to actually take effect. |
|
If you build qt with a namespace, anything linking to Anyway if no feature and triplet only is good for everyone I'm fine with it too. |
This is what I assumed. However, the side effects are not so obvious.
The problem is that forward declarations need to be wrapped by special macros. |
|
I'd like to add that it is really important for me to be able to set the namespace because if you implement a plugin statically linked to qt on mac, you may end up having symbol clashes with the host or other plugins using Qt due to the few objc classes which rely upon a flat namespace, and the only solution I've found is QT_NAMESPACE. |
True, but if they aren't it is a programing mistake. They should be wrapped by So I don't see it as a valid argument to forbid the feature. Also you don't specify |
|
I'm not arguing that supporting Qt's namespace feature is wrong, or that not using the macros is right. The point is:
It is just the combination of feature and triplet variable which has the potential to create bad user experience, and it can be easily avoided. Why not add the triplet variable hint to the port's main description? |
I'm happy with that solution. |
There was a problem hiding this comment.
You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
you should probably deactivate automatic plugin linkage for your plugin instead of adding unnecessary namespaces |
The plugin can't assume anything about the host. This one may very well be using Qt or not. I think |
You don't link plugins to plugins which in the static case really means pulling in all the symbols of one plugin into another. You link plugins to executable and nothing else. Deactivating plugins is done via setting/adding the target property |
JackBoosY
added
the
category:port-feature
If you want to produce a self-contained DSO and Qt is built statically, then yes you link to the plugins as well. |
|
If this namespace is not supported by the upstream, we may not accept this. |
|
https://wiki.qt.io/Qt_In_Namespace |
|
@Neumann-A What do you think about this? |
I still think this should currently be simply an overlay speaks about different versions of qt and dlopen() (not the same version and static libs)
"self-contained" would require Furthermore after reading (Qt unrelated but same problem): |
Do you mean that I can add a configure option to qtbase from a triplet overlay? Or would it imply to maintain a full copy of the port just for this namespace thing?
You missed the point. The plugin interface is host and plugin vendor neutral. Anyone can implement an host or a plugin. No assumptions can be made about what libs are being used by the host. And the host is actually using
I think we have a different understanding of what As far as I understand it, this does not affect the exported symbols. It affects which libraries are being pulled during the link phase of another target depending on the previous one. On linux, I will make every symbols private except my plugin entry by using: I'll explain again the problem with objective-c. Whatever symbol visibility you've set, the objc's runtime will register the objc's classes into a flat namespace. Which means that within a single objc program, you can't have two objc classes with the same name. The only solution is to use prefix/namespace to avoid name clash. If you look at https://github.com/qt/qtbase/blob/dev/config_help.txt#L93 you'll see that We can continue to argue if what I'm doing is correct or not, and honestly if you can provide a better solution I'd be happy to try it. But here are some facts:
|
unlikely. PIMPL and bin compat is the strong suite of Qt.
Your understanding is incomplete. PRIVATE: build requirement; INTERFACE: usage requirement; PUBLIC: both.
So you are building Qt6 two times? One time with namespaces and one time without? vcpkg does not have an inconsistent view on stuff so that is really not needed here.
The given reason is invalid for that conclusion.
Yeah it is useful for vendoring Qt. E.g. If you have a superbuild of paraview and your internal Qt version is different from most systems. Then you can simply vendor Qt yourself add a pv libinfix and do the namespace stuff to get different mangled names. This make sure that your vendored version of Qt gets pulled in and avoids problems with different Qt versions. But that is not the point here. |
I'll give you one scenario:
If the host uses Qt 6.2 and the plugin Qt 6.3, I would not be so sure that everything is going to be fine.
I'd make one triplet for the host and one for the plugins so they can work together without clashing. Consider that the host and plugins are made by different vendors, and when you're going to sell the plugin to a customer, you want to be 100% sure that it won't clash with the host or another plugin, and you want your plugin to be a single file that contains all its dependencies. |
Qt is one of the most strict projects with regard to ABI compatibility. However, there are limitations. I intentionally kept my points short. Offering namespaced Qt via triplet variables is reasonable. However, given that it won't be tested in vcpkg CI, there might be good reasons to be careful with adding this behaviour to the port. |
JackBoosY
added
the
requires:vcpkg-team-review
If that helps I can write additional documentation in the port file, and I can add some logging if the QT_NAMESPACE is being used. We can add a comment that |
|
Qt offers a QT_NAMESPACE define because some users need it. And as for CI testing, that's no different from not testing the various combinations of features that the port is already exposing. |
There was a problem hiding this comment.
This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 2e34a79a814551424d9b1a981b3b110e0a8fbdfd -- versions
./vcpkg x-add-version --allDiff
diff --git a/versions/q-/qtbase.json b/versions/q-/qtbase.json
index d6468a4..b496559 100644
--- a/versions/q-/qtbase.json
+++ b/versions/q-/qtbase.json
@@ -1,7 +1,7 @@
{
"versions": [
{
- "git-tree": "b4ca465db6819d674343bae9c81bdfeca8843bca",
+ "git-tree": "f85e1985bf8c5b27a2ab67cc1fc14b848a09c2b2",
"version-semver": "6.2.2",
"port-version": 4
},You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
There was a problem hiding this comment.
You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
|
It seems that the |
There was a problem hiding this comment.
You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
ras0219-msft
added
requires:author-response
and removed
requires:vcpkg-team-review
There was a problem hiding this comment.
This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 8b62f6fff30c4321945f1e498d85929285d7083a -- versions
./vcpkg x-add-version --allDiff
diff --git a/versions/q-/qtbase.json b/versions/q-/qtbase.json
index b496559..a91de9e 100644
--- a/versions/q-/qtbase.json
+++ b/versions/q-/qtbase.json
@@ -1,7 +1,7 @@
{
"versions": [
{
- "git-tree": "f85e1985bf8c5b27a2ab67cc1fc14b848a09c2b2",
+ "git-tree": "2362188815f81dd666df3eaee1d96a208c1a0838",
"version-semver": "6.2.2",
"port-version": 4
},You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
Thank you very much @ras0219-msft |
There was a problem hiding this comment.
You have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}The following files are affected:
ports/qtbase/portfile.cmake
You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/qtbase/vcpkg.json
Valid values for the license field are listed at https://spdx.org/licenses/
|
Note that qt should behave the same with and without this modification, so this changes should not break the user's expected. |
|
@JackBoosY the issue is flagged has require author's response but I think provided all responses already. I've also applied the suggested change. Should we move forward? Edit: sorry, when I've wrote this message, I didn't your changes above. Maybe the web page needed a refresh... |
|
Thanks!
Under most circumstances, this is exactly the consideration we should have. However, there is always the need for "if the advanced user knows what they are doing, they can tickle XYZ and they're on their own". Options that must be manually set in a custom triplet are sufficiently obscure that they may violate this (and a user that uses it is largely on their own). |
|
Still this is a global variable and thus needs to be prefixed with |
|
Thank you very much everyone. |
Fixes #22712