"This (hopefully) fixes the build failure noticed in the FreeBSD (and some linuxes)" - leaves the question: why should it exactly fix it? :)

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.
So in a highly parallel build the automoc is run before the JSON file is generated.

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})

Thing is that Plasma only uses the kcoreaddons_desktop_to_json macro, so this is why it runs the chance to fail here, while all other projects which create the JSON still on the fly, also call kcoreaddons_add_plugin, cmp. https://lxr.kde.org/search?_filestring=&_string=kcoreaddons_add_plugin&_casesensitive=1

But not further explored, so still curious if that really is the case or if you found the real reason.

Associated bug https://bugs.kde.org/show_bug.cgi?id=389982

In 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.

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. Note, though, that I haven't dissected a Makefile / ninja file to see that the dependency arc is really there. We just tested it by re-running the build a couple of times on 32- and 48-core machines.

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.

I'll try to do some Makefile-dissection.

In D10450#204762, @adridg wrote:

In 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.

To add some code to my words, a proposed fix for the detected unneeded json file and separate cmdl tool dependency now up here: https://phabricator.kde.org/D10485

So what's the conclusion here? Is this only a bug in kcm_lookandfeel or do we think that some follow-on changes are still required in kcoreaddons_desktop_to_json?

I think as @adridg points out that it should be a target, this should go in -- and the @kossebau already committed the proper workaround in D10485, right?

Per my previous comment, I still don't see how changing this to a target would solve anything.

For one, the CMake implementation allows both files and targets to be specified there.

Additionally, this would just add a different kind of dependency to kcm_lookandfeel, but not change lookandfeel's dependencies -- the problem persists if target B depends on a source file that target A depends on, but only target A depends on the generation of a file that this source file depends on.

Yes, I think I agree with @rakuco. Especially since the fix for D10485 ended up being reverted. I still think a separate fix is needed for kcm_lookandfeel, but the issue is that the kcoreaddons_desktop_to_json macro generates a JSON file which is intended for use in a compiled file, and there's no easy way to connect that dependency within *this* macro since the dependent source file is never actually passed into the macro.

I do think it makes more sense to have the custom command here be a custom target but that wouldn't help fix the dependency link-up.

Would it be possible in CMake to alter the macro to somehow make the JSON a dependency of *all* the source files of the provided target instead of just the auto-generated files? That would seem to get the right ordering without too much fuss. add_custom_command has a second signature that looks useful (offers to make the command happen in "PRE BUILD", however it only works for MSVC.

Alternately we could try to do what the kcoreaddons_add_plugin macro does and let the calling code pass in the sources to be made dependent upon the JSON.

What about this? I can't change the diff since I didn't create the RR, but this seems to cause the required dependency rules to be added and works for me to build plasma-desktop. The only real addition is the add_dependencies call. I tried this without the add_custom_target dance but CMake complained about the JSON file not being present so it looks like it needs to be a real CMake target to work.

diff --git a/KF5CoreAddonsMacros.cmake b/KF5CoreAddonsMacros.cmake
index f22817d..c15ab98 100644
--- a/KF5CoreAddonsMacros.cmake
+++ b/KF5CoreAddonsMacros.cmake
@@ -58,13 +58,19 @@ function(kcoreaddons_desktop_to_json target desktop)
       endforeach()
     endif()
 
+    # Create a virtual target for the generated JSON file and force
+    # the passed in target and its auto-generated sources to depend upon it
+    string(RANDOM _target_name_suffix)
+    set(_json_target "desktop_to_json_${_target_name_suffix}")
+    add_custom_target(${_json_target})
     add_custom_command(
-        OUTPUT ${json}
+        TARGET ${_json_target}
         COMMAND ${command}
         WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
         DEPENDS ${desktop}
     )
-    set_property(TARGET ${target} APPEND PROPERTY AUTOGEN_TARGET_DEPENDS ${json})
+    add_dependencies(${target} ${_json_target})
+    set_property(TARGET ${target} APPEND PROPERTY AUTOGEN_TARGET_DEPENDS ${_json_target})
 endfunction()
 
 function(_desktop_to_json_cmake28 desktop json compat)

A resulting Make-based build directory seems to contain the right rules:

CMakeFiles/Makefile2
6522:kcms/lookandfeel/CMakeFiles/kcm_lookandfeel.dir/all: kcms/lookandfeel/CMakeFiles/desktop_to_json_ChFVC.dir/all
6597:kcms/lookandfeel/CMakeFiles/kcm_lookandfeel_autogen.dir/all: kcms/lookandfeel/CMakeFiles/desktop_to_json_ChFVC.dir/all

In D10450#208413, @mpyne wrote:

Yes, I think I agree with @rakuco. Especially since the fix for D10485 ended up being reverted.

Would be happy if anyone on KDE neon could explore why it fails there (only reported for it so far AFAIK) so it had to be reverted. D10607 meanwhile up as a less fragile-with-automoc variant.

I still think a separate fix is needed for kcm_lookandfeel, but the issue is that the kcoreaddons_desktop_to_json macro generates a JSON file which is intended for use in a compiled file, and there's no easy way to connect that dependency within *this* macro since the dependent source file is never actually passed into the macro.

So just to make sure we are all on the same page: for what I have understood meanwhile is what is missing but needed here is a dependency rule between
a) the generated JSON file (kcm_lookandfeel.json)
b) the generated moc file (kcm.moc) created by moc for the source file which references that JSON file in the related QObject subclass declaration and also has the include statement (kcm.cpp)

And this is what

set_property(TARGET ${target} APPEND PROPERTY AUTOGEN_TARGET_DEPENDS ${json})

should ensure at least by what the docs claim IIUC, but somehow does not.

Alternately we could try to do what the kcoreaddons_add_plugin macro does and let the calling code pass in the sources to be made dependent upon the JSON.

All the other source files should not have any influence here, only the cpp file referencing the JSON file.

And adding

add_dependencies(${target} ${_json_target})

would also not ensure that straight dependency between the creation of the JSON file and the time when moc is run, no?

In D10450#208453, @kossebau wrote:

So just to make sure we are all on the same page: for what I have understood meanwhile is what is missing but needed here is a dependency rule between
a) the generated JSON file (kcm_lookandfeel.json)
b) the generated moc file (kcm.moc) created by moc for the source file which references that JSON file in the related QObject subclass declaration and also has the include statement (kcm.cpp)

And this is what

set_property(TARGET ${target} APPEND PROPERTY AUTOGEN_TARGET_DEPENDS ${json})

should ensure at least by what the docs claim IIUC, but somehow does not.

OK, I see what you're talking about.

And adding

add_dependencies(${target} ${_json_target})

would also not ensure that straight dependency between the creation of the JSON file and the time when moc is run, no?

No, you're right, it would only ensure that automoc and JSON creation both happen before the rest of the normal target build occurs.

The custom target has a separate annoyance, using add_custom_target instead of add_custom_command makes it so that the entire JSON generation process seems to happen each time make or ninja is run, rather than only when needed from the dependencies changing, though I think this is due to the add_dependencies call rather than any issue specific to the CMake target itself.

This isn't just a problem on KDE Neon though, is it? I thought FreeBSD is also affected?

For the record, the FreeBSD builds on the CI system hit this fairly regularly.
As shown by https://build.kde.org/view/Plasma/job/Plasma%20plasma-desktop%20kf5-qt5%20FreeBSDQt5.9/

This isn't just a problem on KDE Neon though, is it? I thought FreeBSD is also affected?

To be clear, FreeBSD is affected by not having any fix in the tree (i.e. the json file not being present when moc is invoked), whereas Neon fails with D10485 applied.

OK then, I think @kossebau is right in that this is a dependency issue in the lookandfeel part of plasma-desktop. The kcm_lookandfeel target declares the JSON dependency (with the CMake macro) in time for CMake to care about it and ensure the generated build system picks up the dependency for automoc to run. The lookandfeel command line tool target does not add the dependency on the JSON file and there's no other reason for CMake to know that the JSON file must be generated before lookandfeel's AUTOMOC can complete.

ie. CMake perceives this order as legal:

1. AUTOMOC for lookandfeel
2. moc runs on kcm.cpp
3. JSON generation for AUTOMOC for kcm_lookandfeel
4. kcm_lookandfeel.json is generated
5. AUTOMOC for kcm_lookandfeel
6. moc runs on kcm.cpp
7. build continues

2 and 6 are identical here and it seems that CMake is smart enough to only do this once in the whole build, but CMake only has the dependency on the JSON for the moc at step 6, not step 2, so CMake doesn't know that it also needs to delay lookandfeel's AUTOMOC phase until after JSON generation.

That all said, if D10607 works (and that diff is precisely how I'd suggest handling the issue), then I think we can then abandon this revision. Let me know if you disagree.

I agree this can be abandoned -- whatever solution we agree upon should probably be done in plasma-desktop.

BTW, for the related issue of moc not being re-run on the regeneration of the JSON file, I managed to get some test case to narrow this to automoc possibly and just reported it as https://gitlab.kitware.com/cmake/cmake/issues/17750

And given what I think I learned here; also created D10665 to remove the seemingly effectless rules for depending the source's object file on the JSON file.