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

Proposed by Balló György on 2011-11-24
Status: Merged
Approved by: Didier Roche on 2011-11-29
Approved revision: 392
Merged at revision: 390
Proposed branch: lp:~ballogy/bamf-qt/fix-imports-dir-location
Merge into: lp:bamf-qt
Diff against target: 15 lines (+4/-1)
1 file modified
CMakeLists.txt (+4/-1)
To merge this branch: bzr merge lp:~ballogy/bamf-qt/fix-imports-dir-location
Reviewer Review Type Date Requested Status
Didier Roche Approve on 2011-11-29
Olivier Tilloy 2011-11-24 Approve on 2011-11-29
Review via email: mp+83248@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Olivier Tilloy (osomon) wrote :

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) wrote :

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

Balló György (ballogy) wrote :

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) wrote :

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) wrote :

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) wrote :

Now I applied this change.

Olivier Tilloy (osomon) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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 (didrocks) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2011-11-23 10:00:39 +0000
3+++ CMakeLists.txt 2011-11-29 06:28:35 +0000
4@@ -100,7 +100,10 @@
5 add_custom_target(check)
6
7 # Install
8-set(IMPORT_INSTALL_DIR lib/qt4/imports/bamf)
9+# We don’t refer to QT_IMPORTS_DIR, because it requires at least one QML plugin
10+# installed. Query qmake instead. (see bug #894805)
11+_qt4_query_qmake(QT_INSTALL_IMPORTS qt_imports_dir)
12+set(IMPORT_INSTALL_DIR ${qt_imports_dir}/bamf)
13 set(INCLUDE_INSTALL_DIR include/QtBamf)
14
15 ## QtBamf

Subscribers

People subscribed via source and target branches