Force build order to prevent build happening before ui .h file exists
AbandonedPublic

Authored by dvratil on Mar 17 2019, 4:02 PM.

Details

Reviewers
mlaurent
krop
jriddell
Group Reviewers
KDE PIM
Test Plan

build on neon's 64 core machine

Diff Detail

Repository
R44 KDE PIM Runtime
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9724
Build 9742: arc lint + arc unit
jriddell created this revision.Mar 17 2019, 4:02 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptMar 17 2019, 4:02 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
jriddell requested review of this revision.Mar 17 2019, 4:02 PM

compile failed on our 64 core arm machine

https://build.neon.kde.org/job/bionic_unstable_kde_kdepim-runtime_bin_arm64/118/console

00:32:13 make[4]: Leaving directory '/workspace/build/obj-aarch64-linux-gnu'
00:32:13 CMakeFiles/Makefile2:2525: recipe for target 'resources/kalarm/kalarm/CMakeFiles/kalarmconfig.dir/all' failed
00:32:13 In file included from /workspace/build/resources/kalarm/shared/alarmtyperadiowidget.cpp:22:0:
00:32:13 /workspace/build/resources/kalarm/shared/alarmtyperadiowidget.h:26:10: fatal error: ui_alarmtyperadiowidget.h: No such file or directory
00:32:13 #include "ui_alarmtyperadiowidget.h"
00:32:13 ^~~~~~~~~~~~~~~~~~~~~~~~~~~

ki18n_wrap_ui already marks the file as generated and appends it to the list of source files, so the akonadi_kalarm_resource target should already depend on it, so I'm not entirely convinced this is the real fix....

Perhaps it's a bug in cmake ?

pino added a subscriber: pino.Mar 18 2019, 6:16 AM

It's a simple race condition between two targets:

  • ui_alarmtyperadiowidget.h is included by ../shared/alarmtyperadiowidget.cpp
  • ui_alarmtyperadiowidget.h is built only as part of the akonadi_kalarm_resource target
  • both the akonadi_kalarm_resource, and kalarmconfig targets have ../shared/alarmtyperadiowidget.cpp as source

hence, if ui_alarmtyperadiowidget.h is not generated before the build of the kalarmconfig starts, then the error happens

IMHO this patch does not fix the issue, because nothing will make the build of the kalarmconfig target wait for the generated ui_alarmtyperadiowidget.h.

A simpler solution is to make the kalarmconfig target depend on the akonadi_kalarm_resource one:

add_dependencies(kalarmconfig akonadi_kalarm_resource)
krop added a comment.Mar 18 2019, 8:28 AM

Maybe the 'OBJECT'[1] target type could help here, e.g (builds locally, no idea if it could solve your race):

diff --git a/resources/kalarm/kalarm/CMakeLists.txt b/resources/kalarm/kalarm/CMakeLists.txt
index a57de92bb..5b3a6ca19 100644
--- a/resources/kalarm/kalarm/CMakeLists.txt
+++ b/resources/kalarm/kalarm/CMakeLists.txt
@@ -20,7 +20,6 @@ set(kalarmresource_SRCS
 
 install(FILES kalarmresource.desktop DESTINATION "${KDE_INSTALL_DATAROOTDIR}/akonadi/agents")
 
-ki18n_wrap_ui(kalarmresource_SRCS ../shared/alarmtyperadiowidget.ui)
 kcfg_generate_dbus_interface(${CMAKE_CURRENT_SOURCE_DIR}/kalarmresource.kcfg org.kde.Akonadi.KAlarm.Settings)
 qt5_add_dbus_adaptor(kalarmresource_SRCS
     ${CMAKE_CURRENT_BINARY_DIR}/org.kde.Akonadi.KAlarm.Settings.xml settings.h Akonadi_KAlarm_Resource::Settings icalsettingsadaptor ICalSettingsAdaptor)
@@ -29,7 +28,14 @@ ecm_qt_declare_logging_category(kalarmresource_SRCS HEADER kalarmresource_debug.
 
 add_custom_target(kalarm_resource_xml ALL DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/org.kde.Akonadi.KAlarm.Settings.xml)
 
-add_executable(akonadi_kalarm_resource ${kalarmresource_SRCS})
+# ui_alartyperadiowidget.h is used by both akonadi_kalarm_resource and kalarmconfig
+ki18n_wrap_ui(kalarmresource_shared_SRCS ../shared/alarmtyperadiowidget.ui)
+add_library(akonadi_kalarm_resource_shared OBJECT ${kalarmresource_shared_SRCS})
+
+add_executable(akonadi_kalarm_resource
+  $<TARGET_OBJECTS:akonadi_kalarm_resource_shared>
+  ${kalarmresource_SRCS}
+)
 
 if( APPLE )
     set_target_properties(akonadi_kalarm_resource PROPERTIES MACOSX_BUNDLE_INFO_PLIST ${CMAKE_CURRENT_SOURCE_DIR}/../Info.plist.template)
@@ -54,6 +60,7 @@ install(TARGETS akonadi_kalarm_resource ${KDE_INSTALL_TARGETS_DEFAULT_ARGS})
 ############################## Config plugin #################################
 
 set(kalarmconfig_SRCS
+    $<TARGET_OBJECTS:akonadi_kalarm_resource_shared>
     kalarmconfig.cpp
     ../shared/alarmtyperadiowidget.cpp
     ${kalarmresource_common_SRCS}

[1] https://cmake.org/cmake/help/v3.5/command/add_library.html#object-libraries

In D19830#433446, @cgiboudeaux wrote:

Maybe the 'OBJECT'[1] target type could help here, e.g (builds locally, no idea if it could solve your race):

It successfully compiles with this patch

krop added a comment.Mar 19 2019, 4:44 PM

FTR, the fix was pushed, but now the BSD build is unhappy.

@cgiboudeaux I think a proper fix has now been submitted, right? Can this be closed/abandoned?

dvratil commandeered this revision.Apr 7 2019, 12:31 PM
dvratil abandoned this revision.
dvratil added a reviewer: jriddell.