Merge lp:~zorba-coders/zorba/module-depends into lp:zorba

[David Graf]

Additional optional parameter for DECLARE_ZORBA_MODULE macro to pass targets the declared module depends on.

Additional optional parameter for DECLARE_ZORBA_MODULE macro to pass targets the declared module depends on.

[David Graf (davidagraf)]

Till, this changed is needed for 28msec. Our mongodb module is dependent on one of our targets.

[David Graf (davidagraf)]

review: Approve

[Till Westmann (tillw)]

Looks good to me. Added Chris as the master of all Zorba CMake files as a reviewer.

review: Approve

[Chris Hillery (ceejatec)]

As written, DEPENDS will only do anything if the module includes C++ code. If that is sufficient for your needs, fine, but please change the name of the parameter to something like LIBRARY_DEPENDS to make this clear.

If you need to have targets executed for modules that do NOT have C++ code, then you may be able to make use of the fifth argument to ADD_COPY_RULE(). However, if you go this route, test it *extensively*; adding target dependencies to CMake custom commands (which is necessary for that to work) often leads to really erratic and frustrating CMake behaviour. It won't even have a chance to work on CMake prior to version 2.8.4, either. I do something similar for DECLARE_ZORBA_JAR(), but it has caused no end of grief and I'm not completely sure it's working right all the time even now.

review: Needs Fixing

[David Graf (davidagraf)]

We need the feature to make a module dependent on some header file generation done with zorba. Is the renaming like this ok?

[Chris Hillery (ceejatec)]

No, this naming (and the header comment) still does not convey that this feature is only functional for modules with C++ code. I don't want anyone writing a pure XQuery module to think that this will work for them, because it won't.

I still suggestion something like:

# LIBRARY_DEPENDS - (optional) A list of targets which the external function library will depend on

It's not a great name, but it at least relates it to the LINK_LIBRARIES option which is also only relevant for external function libraries.

[David Graf (davidagraf)]

chris, is it ok like this?

[Chris Hillery (ceejatec)]

Looks like maybe you had a search-and-replace problem. The arguments to PARSE_ARGUMENTS are semicolon-separated, not space-separated, and the constructed variable would be named ${MODULE_LIBRARY_DEPENDS}, not ${MODULE LIBRARY_DEPENDS}.

I've corrected those problems and also expanded the comment for the new parameter. I'm assuming that you have tested the earlier code in Sausalito and that it actually does what you need it to do, so I'll go ahead and vote Approve. If you are OK with my changes, go ahead and mark the proposal Approved to hopefully get it merged.

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/module-depends-2012-10-30T09-34-54.144Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/module-depends into lp:zorba failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job module-depends-2012-10-30T09-34-54.144Z is finished.
  The final status was:

  1 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[David Graf (davidagraf)]

the failing test works on my machine. Seems to be a timeout. I try again ..

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/module-depends-2012-10-30T10-13-48.213Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job module-depends-2012-10-30T10-13-48.213Z is finished. The final status was:

All tests succeeded!

=== modified file 'cmake_modules/ZorbaModule.cmake'
--- cmake_modules/ZorbaModule.cmake	2012-10-08 12:09:36 +0000
+++ cmake_modules/ZorbaModule.cmake	2012-10-30 08:55:24 +0000
@@ -118,6 +118,9 @@
 #              information; see below
 #       TEST_ONLY - (optional) Module is for testcases only and should not
 #              be installed
+#       LIBRARY_DEPENDS - (optional) List of targets that the external
+#              function library will depend on (only works if the module
+#              contains C++ external functions)
 #
 # CONFIG_FILES - any files specific here will be copied to
 # CMAKE_CURRENT_BINARY_DIR using CONFIGURE_FILE(). They may contain
@@ -136,7 +139,7 @@
 # file enough to deduce the URI and version?
 MACRO (DECLARE_ZORBA_MODULE)
   # Parse and validate arguments
-  PARSE_ARGUMENTS(MODULE "LINK_LIBRARIES;EXTRA_SOURCES;CONFIG_FILES"
+  PARSE_ARGUMENTS(MODULE "LINK_LIBRARIES;EXTRA_SOURCES;CONFIG_FILES;LIBRARY_DEPENDS"
     "URI;FILE;VERSION" "TEST_ONLY" ${ARGN})
   IF (NOT MODULE_FILE)
     MESSAGE (FATAL_ERROR "'FILE' argument is required for ZORBA_DECLARE_MODULE()")
@@ -268,6 +271,9 @@
     # the module *URI*'s final component.
     SET(module_lib_target "modlib${num_zorba_modules}_${module_name}")
     ADD_LIBRARY(${module_lib_target} SHARED ${SRC_FILES})
+    IF (MODULE_LIBRARY_DEPENDS)
+      ADD_DEPENDENCIES(${module_lib_target} ${MODULE_LIBRARY_DEPENDS})
+    ENDIF()
     GET_FILENAME_COMPONENT(module_filewe "${module_filename}" NAME_WE)
     IF (MODULE_VERSION)
       # If there's a version, insert it into the module library name