Merging the android plugin to Qt Creator
Daniel Teske
daniel.teske at nokia.com
Fri Jul 22 16:08:36 CEST 2011
Hi,
BogDan recently opened a merge request for the creator changes. So I had a not
very through look through it to figure out what needs to be done before we
could accept the MR.
There's general agreement from the Chief Maintainer that a Android plugin fits
into Creator. I agree.
So, my initial issues list, which need to be fixed before we can consider
merging the code. I concentrated on those parts that are outside of the
android plugin. Don't worry about the lengh of this list, I'll help you
getting it sorted out. Also most of it is for Ryan. Also feel free to come to
the #qt-creator irc channel to discuss specifics.
a) Contributor Agreement
Everyone that has committed to the qt creator android plugin needs to be added
to our internal list of people that have agreed to the contributors agreement,
which means everyone needs to do one MR for qt-creator or qt.
Everyone being:
BogDan Vatra (already done)
mingw-android, Ray Donnelly, Thomas Zander, Marius Bugge Monsen and Espen
Riskedal
b) License
Some files currently have no license header at all
Most of the other files do have the following header:
/*
I BogDan Vatra < bog_dan_ro at yahoo.com >, the copyright holder of this work,
hereby release it into the public domain. This applies worldwide.
In case this is not legally possible, I grant any entity the right to use
this work for any purpose, without any conditions, unless such conditions
are required by law.
*/
Considering that some files do have multiple authors, that needs to be fixed.
You'll need to decide whether you want to have the plugin under the above
license, then all authors must agree to that, or if you want to go with the
standard LGPL. A situation where some parts are by BogDan are in the public
domain and other author's work is not is theoretically possible, but really
not a good option.
c) Target branch
2.3 is already feature frozen, so the target branch should be master.
Currently master merges cleanly into the MR.
d) Changes I'll integrate to master
The commit 43240a9 MinGW compilation fixes for private headers
Exporting Qt4ProjectManager::QMakeStep and Qt4ProjectManager::MakeStep
e) Changes to the android plugin
I have attached a few commits for stuff that was pretty easy to fix:
0001-Remove-wrong-merge-resolution.patch
The code moved in between the merges and is already fixed at the new place.
0002-Remove-accidentally-added-.commit.template.patch
The commit message didn't mention the .commit.tempalte, I assumed it was
commited in error. I could also add the .commit.template to master if you want
that.
0003-Remove-unecessary-whitespace-changes.patch
0004-Unecessary-change-to-import-code.patch
The code there is only used for importing .user files that are pre 2.0, as such
the code change should be unecessary.
0005-Simplfy-template-code.patch
0006-Reformat-deployment.pri-to-be-more-readeable.patch
0007-Bump-stub-version-of-template.patch
Those 3 patches change the template to what I think is better. Also see
comments below.
0008-Remove-settign-of-ANDROID_NDK_HOST-from-necessitas.b.patch
See commit message.
Feel free to submit them to the applicable branch.
f) mingw/*
Those files need to be removed from the MR for hopefully obvious reasons.
g) macdeployqt/*
============= TODO look at the patches
There is unfourtanetly no maintainer for macdeployqt, but the patches should
be made against the macdeployqt in qt.
h) Changes in src/tools/qpatch
After a quick discussion at the office we removed src/tools/qpatch completely
from master since it is neither used by the stand alone creator packages nor
by the sdk and is known to be buggy.
I'd recommend instead of patching and using qpatch to use the facilities in
the QtSDK found in installer-framework/installerbuilder/libinstaller/qtpatch
and installerbuilder/libinstaller/qtpatchoperation.cpp and then backing out
the changes to qpatch
i) build.sh
We don't want that as part of the creator repository. As far as we are
concerned building creator should be done in a normal cmd shell on windows and
not in a msys shell.
j) bin/necessitas and bin/necessitas.bat
The setting of ANDROID_NDK_HOST seemed to be a left over from before setting
it in androidtoolchain.cpp, so I removed it, see attached patch.
We used to have a similar LD_LIBRARY_PATH setting script on linux but after
some internal discussions we removed it. On windows
k) Settings Path and Renaming to Necessitas
We think if we merge the android plugin into creator, then there is no point
in renaming Qt Creator and no point in using a different settings path. The sdk
team also wants to produce "one" sdk that can install components from different
providers.
If you do want to have a different name/settings path, then there are quite a
few places which need to be fixed and the default should be "Qt Creator" not
"Necessitas Qt Creator"
l) qtcreator.rc
I don't understand the need to change the file and the commit mesage doesn't
explain it either: "Fix errors in qtcreator.rc by saving it from Visual
Studio"
We are compiling qt creator with visual studio and have no problems at all
with qtcreator.rc, so could you explain why you need this change?
m) qmakeStep::moreArguments()
Adding the "-win32" to the qmake command line. As far as I understand you want
to have qmake run in HOST_WIN_MODE on windows. But that is the default if
qmake is compiled for windows.
n) lowerCasing in qt4buildconfiguration.cpp
#ifdefing out the lowercasing of specs
"Don't do the toLower stuff on lots of paths when Q_OS_WIN and __MINGW32__ is
defined. This is an aesthetic thing. I don't like showing all pahs in lower
case."
The paths are lowercased to compare them against each other, without the
lowercasing the comparision will fail for paths that are the same. Thus since
the change is just for aesthetics, I'd back it out again.
o) profileeavluator.cpp
The profileevaluator in creator should behave the same as the one in qmake.
And the evaluator in qmake lowercases the first device letter (even if that's
not correct) and uses "\" on windows. I guess the drive letter change is just
for asthetics? What does the path separator break? Does that have something to
do with msys?
p) debugger plugin
The debbuger plugin has quite a few changes, some of them seem to be hacks.
Also I'm not that well versed in the debugger plugins code, though I discussed
a earlier diff with the debugger maintainer (Andre).
I'm somewhat confussed whether you are using the python dumpers, or not.
So, the changes:
Adding "arm-linux-androideabi" to gnuTargets: fine
soLibSearchPath: in general looks fine, andre mentioned some details he wanted
to see fixed
Always offering all toolchains in DebuggerToolChainComboBox::init
The commit message already classifies that as a hack, I just don't understand
why it was needed. The check for hostAbi.os() == abi.os() should be true for
msys toolchains too. Or is .os() broken for msys toolchains?
Disabling setting PYTHONPATH. Why is that needed? Note that the code that is
disabled was rewritten a lot since you disabled it.
q) abi.h abi.cpp
The change looks good, but can't be merged without the rest of the android
plugin.
r) templates/qtquickapp/*
The changes looked almost okay. I didn't understand the setBaseUrl() change,
nor can I see how that makes sense. I reformatted some of the code for better
readability and added a bumping of the template version. (Projects with prior
template versions are automatically updated then.) See the attached patches.
s) main.cpp #ifdef
There is a change in main.cpp changing the plugin search paths to be only
searched on the "matching" platform. The commit message is:
"Mac OS X: Fix for case sensitive filesystems (had mixed plugins/PlugIns,
always uses PlugIns now)"
Ray can you explain what that commit fixes? That sounds like something I could
cherry-pick to master.
t) main.cpp myMessageOutput
The main motivation for that seems to be:
"qInstallMsgHandler so that asserts are not fatal"
We like Q_ASSERTS to be fatal, we do have a separate macro QTC_ASSERT for
cases where a non fatal handling is possible/desired.
u) Settings SystemScope to UserScope
Quite a few settings (if not all) are using the UserScope instead of the
SystemScope. Why is that needed?
I might have missed a few changes here and there.
I'm looking forward to working on that. :)
daniel
--
Daniel Teske
Software Engineer
Nokia, Qt Development Frameworks
Nokia gate5 GmbH
Firmensitz: Invalidenstr. 117, 10115 Berlin, Germany
Registergericht: Amtsgericht Charlottenburg, Berlin: HRB 106443 B
Umsatzsteueridentifikationsnummer: DE 812 845 193
Geschäftsführer: Dr. Michael Halbherr, Karim Tähtivuori
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-wrong-merge-resolution.patch
Type: text/x-patch
Size: 845 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Remove-accidentally-added-.commit.template.patch
Type: text/x-patch
Size: 1812 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0001.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Remove-unecessary-whitespace-changes.patch
Type: text/x-patch
Size: 3213 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0002.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Unecessary-change-to-import-code.patch
Type: text/x-patch
Size: 1721 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0003.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Simplfy-template-code.patch
Type: text/x-patch
Size: 1481 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0004.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Reformat-deployment.pri-to-be-more-readeable.patch
Type: text/x-patch
Size: 1909 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0005.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-Remove-settign-of-ANDROID_NDK_HOST-from-necessitas.b.patch
Type: text/x-patch
Size: 776 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0006.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Bump-stub-version-of-template.patch
Type: text/x-patch
Size: 1290 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/necessitas-devel/attachments/20110722/df8d8c91/attachment-0007.patch
More information about the Necessitas-devel
mailing list