Merge lp:~ballogy/bamf-qt/fix-imports-dir-location into lp:bamf-qt

[Balló György]

This change replace the hard-coded imports dir location with QT_IMPORTS_DIR variable. I need it for Arch Linux, because it uses /usr/lib/qt/imports instead of /usr/lib/qt4/imports

[Olivier Tilloy (osomon)]

The change looks sound.
However I tested it on Ubuntu and it appears that cmake has a bug whereby QT_IMPORTS_DIR is not correctly defined (I reported it as bug #894805).
Therefore it cannot be merged as is. A possible solution would be to check for the value of ${QT_IMPORTS_DIR} and if it’s invalid (e.g. "QT_IMPORTS_DIR-NOTFOUND") fallback on the hardcoded value /usr/lib/qt4/imports. What do you think?

[Balló György (ballogy)]

OK, I added a fallback to lib/qt4/imports/bamf if QT_IMPORTS_DIR not found.

[Balló György (ballogy)]

I found that FindQt4.cmake checks if 'Qt' directory exists in '/usr/lib/qt4/imports/' path. But on Ubuntu, it's probably exists only if one of the following QML plugins installed:
- libqt4-declarative-folderlistmodel
- libqt4-declarative-gestures
- libqt4-declarative-particles
- libqt4-declarative-shaders

What do you think about query qmake directly? It should works even if no QML plugins installed:
_qt4_query_qmake(QT_INSTALL_IMPORTS qt_imports_dir)
set(IMPORT_INSTALL_DIR ${qt_imports_dir}/bamf)

[Olivier Tilloy (osomon)]

Thanks for the investigation György! I have updated the bug report with your findings.

Your proposal to query qmake directly sounds perfect to me, please go ahead and do this!

[Olivier Tilloy (osomon)]

Note that I’d add a comment explaining why we don’t refer to QT_IMPORTS_DIR and we query qmake instead, with a link to the bug report.

[Balló György (ballogy)]

Now I applied this change.

[Olivier Tilloy (osomon)]

Thanks György.
There’s a couple more issues with that change though:

 - IMPORT_INSTALL_DIR should be a relative path, just like INCLUDE_INSTALL_DIR, i.e. the /usr/ prefix should somehow be truncated in a clever way. This is needed so that installing to a different prefix works.

 - debian/libqtbamf1.install should be updated too (it should be generated), so that the path to the QML plugin is not hardcoded.

I understand that your interests lie in making a package for ArchLinux, so I can take care of the second point later on, but the first point needs to be addressed. Thanks!

review: Needs Fixing

[Balló György (ballogy)]

I don't know how to truncate the path, but it's really required to be relative? I think that it's a Qt plugin, and therefore it should be installed into the same place where Qt installed. E.g. appmenu-qt uses the similar QT_PLUGINS_DIR variable, which is also an absolute patch and cannot be prefixed:
http://bazaar.launchpad.net/~agateau/appmenu-qt/trunk/view/head:/src/CMakeLists.txt#L37

[Olivier Tilloy (osomon)]

> I don't know how to truncate the path, but it's really required to be
> relative? I think that it's a Qt plugin, and therefore it should be installed
> into the same place where Qt installed. E.g. appmenu-qt uses the similar
> QT_PLUGINS_DIR variable, which is also an absolute patch and cannot be
> prefixed:
> http://bazaar.launchpad.net/~agateau/appmenu-
> qt/trunk/view/head:/src/CMakeLists.txt#L37

You have a point here!
I guess in this situation having an absolute path is fine. Let’s merge it as is then!

review: Approve

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-bamf-qt/2/console reported an error when processing this lp:~ballogy/bamf-qt/fix-imports-dir-location branch.
Not merging it.

[Didier Roche-Tolomelli (didrocks)]

sorry Olivier, I was debugging bamf ain the chroot and so, didn't notice this merge. Ignore it :)

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2011-11-23 10:00:39 +0000
+++ CMakeLists.txt	2011-11-29 06:28:35 +0000
@@ -100,7 +100,10 @@
 add_custom_target(check)
 
 # Install
-set(IMPORT_INSTALL_DIR lib/qt4/imports/bamf)
+# We don’t refer to QT_IMPORTS_DIR, because it requires at least one QML plugin
+# installed. Query qmake instead. (see bug #894805)
+_qt4_query_qmake(QT_INSTALL_IMPORTS qt_imports_dir)
+set(IMPORT_INSTALL_DIR ${qt_imports_dir}/bamf)
 set(INCLUDE_INSTALL_DIR include/QtBamf)
 
 ## QtBamf