<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/126161/">https://git.reviewboard.kde.org/r/126161/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 25th, 2015, 8:39 a.m. CET, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/126161/diff/1/?file=418134#file418134line1621" style="color: black; font-weight: bold; text-decoration: underline;">src/kdeinit/kinit.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static QString findSharedLib(const QString &lib)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1586</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> See "CoreFoundation and fork()" at http://developer.apple.com/releasenotes/CoreFoundation/CoreFoundation.html</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1619</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> This probably cancels the whole idea of preloading libraries, but it is as it is.</span></pre></td>
</tr>
</tbody>
</table>
<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 if you have to run a separate process which will then dlopen the kdeinit module, the whole purpose of kdeinit is moot. You might as well simplify your life by getting rid of most of the Q_OS_OSX code in this file and simply acting as if the kdeinit module doesn't exist, on Q_OS_OSX.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The existing code to fallback to executing the binary directly will do exactly the same as your generic proxy, possibly even faster (no dlopen).</p></pre>
</blockquote>
<p>On November 25th, 2015, 4:26 p.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;">You're undoubtedly right, I considered doing this myself. It would amount to making the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">--no-fork</code> option the default, no?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't feel comfortable/ready for that right now, without having had a working equivalent to (the patched) kdeinit4 out there in the wild for observation. Can we leave your suggestion for a 2nd round of housekeeping and cleanup?</p></pre>
</blockquote>
<p>On November 25th, 2015, 4:43 p.m. CET, <b>David Faure</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;">Not at all, --no-fork is about kdeinit's own startup, not about the way it starts other applications.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In general I don't see much purpose in pushing complex code if we confirm it to serve no purpose at all.
But I looked a bit further into it, and in fact kinit's launch() does fork+dlopen or fork+exec, depending on whether the kdeinit module was found.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So if fork + exec is a problem on OSX, then indeed that needs to be addressed
But if your patch does fork + exec_helper + dlopen, then that is unnecessarily complex.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The simple version of what I have in mind is http://www.davidfaure.fr/2015/kinit.osx.cpp.diff i.e. never using QLibrary on OSX. Would that work?</p></pre>
</blockquote>
<p>On November 25th, 2015, 5:10 p.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;">Well, all I can say is that with that patch nothing crashes, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kdeinit5 --kded</code> still launches kded5 but as a result I now no longer see something like (in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ps</code> output)</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%">12980 1 400c 0 33 0 2510184 6716 - Ss 0 ?? 0:00.03 /opt/local/bin/kdeinit5 --suicide --nofork
12981 12980 4004 0 48 0 2641864 14856 - S 0 ?? 0:00.12 /opt/local/libexec/kde5/kf5/klauncher --fd=9 libkdeinit5_klauncher
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">but</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%">13211 1 400c 0 33 0 2527592 6724 - Ss 0 ?? 0:00.02 /opt/local/bin/kdeinit5 --suicide --nofork
13225 1076 4006 0 31 0 2432948 576 - S+ 0 ttys004 0:00.00 fgrep kdeinit5
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kwrapper5 /path/to/kwrite</code> now longer launches an kwrite process that can be killed via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">killall kwrite</code> or equivalent. All that is probably to be expected, but that latter observation does feel like a regression of sorts to me.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BTW, I noticed that kded5 will have to be turned into an agent too, it has no business showing up in the Dock.</p></pre>
</blockquote>
<p>On November 25th, 2015, 5:24 p.m. CET, <b>David Faure</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, killall only works on linux because of the proc_settitle stuff.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think my approach would "fix" that "regression" for killall kwrite because it would be a real fork'ed+exec'ed process.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You are seeing the drawbacks of the kdeinit mechanism (e.g. killall, and probably what the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ps</code> entry looks like for kwrite, too) without benefiting from its advantages (faster startup), if you have to go through a helper process.</p></pre>
</blockquote>
<p>On November 25th, 2015, 5:45 p.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;">Erm we have a communications problem here.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No, with "my" fix, applications started through kwrapper appear as individual entries in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ps</code> listings, with your fix only the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kwrapper5 /path/to/command</code> entry shows up.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, if your fix does a "real fork + exec", how come it doesn't run afoul of the limitations imposed on that on OS X? Only because it doesn't actually load <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">l</code> (the result of QLibrary(libpath)), meaning the crash will return the day kdeinit5 itself does something off-limits? The helper from my fix is probably much less likely to develop such symptoms. And even if it does (through Qt) it would be much easier to cure (make it not use Qt at all but only POSIX APIs).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looking at this more closely I do think that my fix could borrow from yours, and skip the whole <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QLibrary l(libpath)</code> bit (including creating <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">l</code>) because that is being done for nothing. For the rest, using a helper does seem to give a better "emulation" of what <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kdeinit5</code> does on Linux, no?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">With "my" fix:</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%">17906 1 400c 0 33 0 2527592 6744 - Ss 0 ?? 0:00.02 /opt/local/bin/kdeinit5 --suicide --nofork
17907 17906 4004 0 33 0 2641356 14844 - S 0 ?? 0:00.10 /opt/local/libexec/kde5/kf5/klauncher --fd=9
505 17925 1096 4006 0 33 0 2491072 5188 - S+ 0 ttys006 0:00.01 kwrapper5 /Applications/MacPorts/KDE4/kwrite.app/Contents/MacOS/kwrite
505 17926 17906 400c 0 48 0 4831548 47552 - S 0 ?? 0:01.00 /Applications/MacPorts/KDE4/kwrite.app/Contents/MacOS/kwrite
</pre></div>
</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On November 25th, 2015, 5:19 p.m. CET, 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 KDE Frameworks.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Nov. 25, 2015, 5:19 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kinit
</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 addresses several issues with the OS X adaptations:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
-2 builds the relevant applications <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">nongui</code> instead of as app bundles
-3 turns klauncher into an "agent" by setting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">LSUIElement</code> to true programmatically
-4 ports a patch that has been in MacPorts' <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">port:kdelibs4</code> since October 14th 2009, which prevents a kdeinit crash that is caused by calling exec after <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">fork()</code> in an application that has used non-POSIX APIs and/or calling those APIs in the exec'ed application. This patch (originally by MacPorts developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a proxy application to do the actual exec.</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 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 . With this patch, starting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kded5</code> will launch kdeinit5 and klauncher5 as expected, but <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kdeinit5 --kded</code> does not yet launch <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kded5</code>. This is probably acceptable for typical KF5 use on OS X (kded5 can be launched as a login item or as a LaunchAgent) but I will have another look at why the kded isn't started.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am not yet able to perform further testing; practice will for instance have to show whether point 2 above needs revision (apps that need to be installed as app bundles).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Similarly it will have to be seen whether point 3 above has any drawbacks. Applications running as agents do not show up in the Dock or App Switcher. Thus, klauncher will not be able to "turn itself into" an application that does have a full GUI presence with my current modification. I don't know if that's supposed to be possible though.
NB: I have been building the KDE4 klauncher in a way that makes it impossible to construct a GUI at all, so I'm not expecting issues in KF5 as long as klauncher's role hasn't evolved too much.</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>src/klauncher/klauncher.h <span style="color: grey">(e155f72)</span></li>
<li>src/start_kdeinit/CMakeLists.txt <span style="color: grey">(46d6cb3)</span></li>
<li>src/kdeinit/kinit.cpp <span style="color: grey">(a18008a)</span></li>
<li>src/kdeinit/CMakeLists.txt <span style="color: grey">(f94db71)</span></li>
<li>src/wrapper.cpp <span style="color: grey">(95b7ec2)</span></li>
<li>src/klauncher/klauncher.cpp <span style="color: grey">(8b3d343)</span></li>
<li>src/klauncher/klauncher_main.cpp <span style="color: grey">(f69aaa5)</span></li>
<li>src/klauncher/CMakeLists.txt <span style="color: grey">(746edfa)</span></li>
<li>src/kdeinit/kdeinit5_proxy.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126161/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>