<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127638/">https://git.reviewboard.kde.org/r/127638/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 12th, 2016, 11:31 p.m. CEST, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-2</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the ide without the plugins is unusable, this patch makes zero sense to me.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">also note that you can already disable sub folders if you really want to do that (I doubt it). Just look at the cache via ccmake e.g.:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span></span> BUILD_TESTING ON
BUILD_clang ON
BUILD_cmake ON
BUILD_cmakebuilder ON
BUILD_custom-buildsystem ON
BUILD_custom-definesandinclude ON
BUILD_custommake ON
BUILD_executeplasmoid ON
BUILD_ghprovider ON
BUILD_kdeprovider ON
BUILD_kdev_includepathsconvert ON
BUILD_kdev_makefileresolver OFF
BUILD_kdev_msvcdefinehelper OFF
BUILD_manpage ON
BUILD_plugins ON
BUILD_qmake ON
BUILD_qmake_parser OFF
BUILD_qmakebuilder ON
BUILD_qmljs ON
BUILD_qthelp ON
</pre></div>
</p></pre>
</blockquote>
<p>On April 13th, 2016, 12:54 a.m. CEST, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Agreed, I don't see the value in the patch either.</p></pre>
</blockquote>
<p>On April 13th, 2016, 10:18 a.m. CEST, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Have you even looked at what this does exactly? </p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the ide without the plugins is unusable</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">makes it possible</em> for someone <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">who has a reason to do so</em> to build the IDE with <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</em> plugins <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">except</em> the kdev-clang plugin. That leaves all other plugins, so the IDE should remain usable for everything that doesn't require libclang.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch is intended as a convenience for packagers. It was <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> designed to allow installing the IDE <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">without</em> ever installing the corresponding kdev-clang plugin, only to be able to build and package the IDE and the plugin separately. With the emphasis on <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">build and</em>; in case it still isn't clear not all packaging/distribution systems support creating multiple packages from a single build.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do I have to understand that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-DKDEVELOP_BUILD_CLANG_PARSER=OFF</code> is equivalent to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-DBUILD_clang=OFF</code>? That isn't apparent from the CMake files.</p></pre>
</blockquote>
<p>On April 13th, 2016, 1:30 p.m. CEST, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, you can use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-DBUILD_*=OFF</code> to disable features. It works for all others, so I suggest you fix your packaging system if it doesn't work. Worst-case, ship this patch there if you really need it. I don't want to maintain that in our code base.</p></pre>
</blockquote>
<p>On April 14th, 2016, 12:56 p.m. CEST, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It doesn't look there's a way to build only kdev-clang with that approach which isn't an option anyway because -DBUILD_clang=OFF does NOT disable the required dependency on clang.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Let me ask something else just to know exactly how I might fix the (<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">NOT</em> <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">mine</em>) packaging system:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">how is the standalone-app-bundle build going to be implemented in the cmake files? Will it be a hardcoded choice (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">if(APPLE)</code>) like the marble project currently does it? Or will it be under proper control of an option variable? That should be hardly any more complicated (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">option(APPLE_BUILD_APPBUNDLE,"build as standalone app bundle",YES)</code> and then <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">if(APPLE_BUILD_APPBUNDLE)</code> for everything that's only related to install locations and not additional link properties). But the present patch isn't exactly complicated either ...</p></pre>
</blockquote>
<p>On January 9th, 2017, 9:12 a.m. CET, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It doesn't look there's a way to build only kdev-clang with that approach which isn't an option anyway because -DBUILD_clang=OFF does NOT disable the required dependency on clang.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then do:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span></span>if (NOT BUILD_clang)
find_package(...)
...
</pre></div>
</p></pre>
</blockquote>
<p>On January 9th, 2017, 9:41 a.m. CET, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or, you move the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">find_package(Clang ...)</code> logic into languages/clang/CMakeLists.txt so that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_optional_add_subdirectory(clang)</code> can indeed be turned off with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-DBUILD_clang=OFF</code> on the commandline. I'd appreciate if that could be done, btw.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other than that this more or less what I'm already doing, with a different option. But skipping the clang parser build is only part of what I'm still doing in my packaging scripts, which also need to be able to build only the clang parser.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The alternative would be to patch languages/clang/CMakeLists.txt so that it can act as a standalone project, something I'll explore when the maintenance load of my current patch becomes too high.</p></pre>
</blockquote>
<p>On January 9th, 2017, 11:04 a.m. CET, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The alternative would be to patch languages/clang/CMakeLists.txt so that it can act as a standalone project, something I'll explore when the maintenance load of my current patch becomes too high.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unlikely that we're going to accept that either (just to make sure you don't waste time asking us to review that). You'd need to copy-paste lots of CMake setup code (cf. head of kdevelop.git:CMakeLists.txt) to get this done. => Maintenance burden.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As said earlier. We/I don't see a lot of value in this patch either, and we don't want to maintain it upstream.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No other distro has problems with the libclang dependency.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It would definitely make less sense to incorporate such a patch upstream, but in terms of maintenance burden it should be easier. I think the requirements of the clang plugin are less likely to change very often, while the patch to make the entire build system build only that plugin can require refactoring much more frequently. Again in terms of maintenance burden my current patch shouldn't be hard to maintain upstream at all given that the source tree is well organised (it just requires a bit of discipline, but you already have that ;)) but I've decided not to insist quite a while ago already.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to other distros most probably either have support for creating multiple packages from a single build and use that for KDevelop and Clang OR they build against a single central clang install (which can evolve independently) OR don't care Arch/Gentoo-style about simply building everything for every install. OR they never bothered bringing the topic up. Only the last 2 alternatives are options for me.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On April 12th, 2016, 6:34 p.m. CEST, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X and KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated April 12, 2016, 6:34 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch is in reaction to a discussion on kdevelop-devel about weakening KDevelop's dependency on a specific Clang (libclang) version.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch introduces 2 CMake options:
- <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDEVELOP_BUILD_IDE</code> builds just the IDE, without the clang-based parser (formerly kdev-clang)
- <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDEVELOP_BUILD_CLANG_PARSER</code> builds just the clang-based parser</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Nothing changes when both options are on (the default): all of KDevelop is built. When both options are off, an error is raised.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The idea is that this should make it a little bit easier for distributions/packaging systems and users to change the clang toolchain.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I presume that the languages subdirectory could be built first or last (that would the patch a bit simpler)?</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On OS X and Linux, against Qt 5.6.0, FWs 5.20.0, prefix=/opt/local</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(2d3faa7)</span></li>
<li>languages/CMakeLists.txt <span style="color: grey">(4e2fde2)</span></li>
<li>languages/clang/CMakeLists.txt <span style="color: grey">(c19a951)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127638/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>