kossebau added a subscriber: kfunk.
kossebau added a comment.

  In D10450#204762 <https://phabricator.kde.org/D10450#204762>, @adridg wrote:
  
  > In D10450#204623 <https://phabricator.kde.org/D10450#204623>, @kossebau 
wrote:
  >
  > > "This (hopefully) fixes the build failure noticed in the FreeBSD (and 
some linuxes)" - leaves the question: why should it exactly fix it? :)
  >
  >
  > 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 **file**, but a **target**. This patch adds a target, created by the given 
command and hooks that into the dependency graph.
  
  
  Okay, by reading the docs I would agree.
  
  >> 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.
  > 
  > 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.
  
  Seems I am to blame for reading code with preoccupied mind :) indeed missed 
what the actual intention of the very line
  
    set_property(TARGET ${target} APPEND PROPERTY AUTOGEN_TARGET_DEPENDS 
${json})
  
  was.
  
  >> Something which e.g. is tried to be solved by the code in the 
`kcoreaddons_add_plugin` macro, by grepping over all the source files to find 
the cpp file which references the JSON file and then create the dependency by
  >> 
  >>   set_property(SOURCE ${dependent_sources} APPEND PROPERTY OBJECT_DEPENDS 
${json})
  >>    
  > 
  > If this patch fixes the dependency arc, then possibly that (expensive?) 
grepping around isn't needed either.
  
  Indeed. I remember vaguely though there were some bugs with 
AUTOGEN_TARGET_DEPENDS, @kfunk didn't you do something related to that?
  
  > I'll try to do some Makefile-dissection.
  
  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:
  
  the CMakeLists.txt of the lookandfeel kcm actually defines two targets, which 
both have as source the 'kcm.cpp' file, 'kcm_lookandfeel' and 'lookandfeeltool`:
  
    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})
  
  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.
  
  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.
  
  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.
  
  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.

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10450

To: tcberner, #freebsd, mpyne, bshah, dfaure
Cc: kfunk, adridg, kossebau, #frameworks, michaelh

Reply via email to