<table><tr><td style="">kossebau added a subscriber: kfunk.<br />kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D10450" rel="noreferrer">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D10450#204762" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10450#204762</a>, <a href="https://phabricator.kde.org/p/adridg/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@adridg</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D10450#204623" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10450#204623</a>, <a href="https://phabricator.kde.org/p/kossebau/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@kossebau</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>"This (hopefully) fixes the build failure noticed in the FreeBSD (and some linuxes)" - leaves the question: why should it exactly fix it? :)</p></div>
</blockquote>
<p>From reading the CMake documentation, it seems (but I'll admit that fancy dependency stuff is a dark art in CMake) that AUTOGEN_TARGET_DEPENDS shouldn't be a <strong>file</strong>, but a <strong>target</strong>. This patch adds a target, created by the given command and hooks that into the dependency graph.</p></div>
</blockquote>
<p>Okay, by reading the docs I would agree.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>From discussion of last week on irc, it seemed that the actual problem is that the generated make files do not contain the dependency between the JSON file that needs to be generated and automoc running over the cpp source file to generate the moc file based on the referenced JSON file.</p></blockquote>
<p>And that's the whole problem, isn't it. If you force the dependency arc to be there -- i.e. by using the target property here -- then it works.</p></blockquote>
<p>Seems I am to blame for reading code with preoccupied mind :) indeed missed what the actual intention of the very line</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">set_property(TARGET ${target} APPEND PROPERTY AUTOGEN_TARGET_DEPENDS ${json})</pre></div>
<p>was.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Something which e.g. is tried to be solved by the code in the <tt style="background: #ebebeb; font-size: 13px;">kcoreaddons_add_plugin</tt> macro, by grepping over all the source files to find the cpp file which references the JSON file and then create the dependency by</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">set_property(SOURCE ${dependent_sources} APPEND PROPERTY OBJECT_DEPENDS ${json})</pre></div></blockquote>
<p>If this patch fixes the dependency arc, then possibly that (expensive?) grepping around isn't needed either.</p></blockquote>
<p>Indeed. I remember vaguely though there were some bugs with AUTOGEN_TARGET_DEPENDS, <a href="https://phabricator.kde.org/p/kfunk/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@kfunk</a> didn't you do something related to that?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>I'll try to do some Makefile-dissection.</p></blockquote>
<p>Tried as well myself to get an idea, though somehow with the patch I still could not find the dep rules I expected. Discovered something else though:</p>
<p>the CMakeLists.txt of the lookandfeel kcm actually defines two targets, which both have as source the 'kcm.cpp' file, 'kcm_lookandfeel' and 'lookandfeeltool`:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">set(kcm_lookandfeel_SRCS
kcm.cpp
../krdb/krdb.cpp
../cursortheme/xcursor/cursortheme.cpp
../cursortheme/xcursor/xcursortheme.cpp
)
add_library(kcm_lookandfeel MODULE ${kcm_lookandfeel_SRCS})
kcoreaddons_desktop_to_json(kcm_lookandfeel "kcm_lookandfeel.desktop" SERVICE_TYPES kcmodule.desktop)
set(lookandfeeltool_SRCS
lnftool.cpp
kcm.cpp
../krdb/krdb.cpp
../cursortheme/xcursor/cursortheme.cpp
../cursortheme/xcursor/xcursortheme.cpp
)
add_executable(lookandfeeltool ${lookandfeeltool_SRCS})</pre></div>
<p>Also note that automoc will be run on the sources for both targets. While the kcoreaddons_desktop_to_json macro as used is just adding rules related to the JSON file for the kcm_lookandfeel target.</p>
<p>Now if we have a very close look at the build log files, we can see that it is actually the automoc being run for the sources of the lookandfeeltool target, which fails due to the missing JSON file. Which makes sense, given there are no dependency rules set by us. So even with this patch we have a race condition on the parallel build for the kcm_lookandfeel. This might also explain why we only see this kind of failure for this kcm, but nowhere else.</p>
<p>So a separate fix for the build issue would be to make sure the K_PLUGIN_FACTORY_WITH_JSON call is in a separate source file only used for the kcm_lookandfeel target.</p>
<p>For this very patch, I am still lost on how cmake generates the rules for automoc stuff, so no clue if switching to an explicit intermediate target improves something.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R244 KCoreAddons</div></div></div><br /><div><strong>BRANCH</strong><div><div>master</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10450" rel="noreferrer">https://phabricator.kde.org/D10450</a></div></div><br /><div><strong>To: </strong>tcberner, FreeBSD, mpyne, bshah, dfaure<br /><strong>Cc: </strong>kfunk, adridg, kossebau, Frameworks, michaelh<br /></div>