[CMake] Make the bindir and libdir arguments to set_output_directory optional

[john.brawn]

Summary

When building a plugin against an installed LLVM toolchain using add_llvm_loadable_module (in the documented manner) doesn't work as nothing sets the *_OUTPUT_INTDIR variables causing an error when set_output_directory is called. Making those arguments optional (causing the default output directory to be used) fixes this.

[beanz]

cmake/modules/AddLLVM.cmake

206 ↗

(On Diff #35877)

I think that a better way to do this would be to use cmake_parse_arguments and make output and bind directory be single value options. Then if one isn't specified it is ignored.

I also think it would be cleaner if set_target_properties were only called if the corresponding value is set.

[chapuni]

LGTM, as a step.

That said, I think set_output_directory may be rewritten.
It may be dedicated to set outdir along LLVM_*_OUTPUT_INTDIR, or reset outdir.

[john.brawn]

cmake/modules/AddLLVM.cmake

206 ↗

(On Diff #35877)

I had thought of using cmake_parse_arguments, but decided against it as it would involve changing how set_output_directory is called. It's only called in three places though, so actually that won't be too much trouble. I'll give it a go.

[john.brawn]

I've gone with the suggestion of using cmake_parse_arguments. I haven't gone for calling set_target_properties is the argument is set, instead of the check at the start, mainly as I'm not sure how moddir would be handled if that were done.

[beanz]

I think all you need to do for handling unset values is something like this:

diff --git a/cmake/modules/AddLLVM.cmake b/cmake/modules/AddLLVM.cmake
index 993bc35..63047e6 100644
--- a/cmake/modules/AddLLVM.cmake
+++ b/cmake/modules/AddLLVM.cmake
@@ -211,17 +211,33 @@ function(set_output_directory target)
   if(NOT "${CMAKE_CFG_INTDIR}" STREQUAL ".")
     foreach(build_mode ${CMAKE_CONFIGURATION_TYPES})
       string(TOUPPER "${build_mode}" CONFIG_SUFFIX)
-      string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} bi ${ARG_BINDIR})
-      string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} li ${ARG_LIBDIR})
-      string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} mi ${moddir})
-      set_target_properties(${target} PROPERTIES "RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${bi})
-      set_target_properties(${target} PROPERTIES "ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${li})
-      set_target_properties(${target} PROPERTIES "LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${mi})
+      if(ARG_BINDIR)
+        string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} bi ${ARG_BINDIR})
+        set_target_properties(${target} PROPERTIES "RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${bi})
+      endif()
+
+      if(ARG_LIBDIR)
+        string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} li ${ARG_LIBDIR})
+        set_target_properties(${target} PROPERTIES "ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${li})
+      endif()
+
+      if(moddir)
+        string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} mi ${moddir})
+        set_target_properties(${target} PROPERTIES "LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${mi})
+      endif()
     endforeach()
   else()
-    set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${ARG_BINDIR})
-    set_target_properties(${target} PROPERTIES ARCHIVE_OUTPUT_DIRECTORY ${ARG_LIBDIR})
-    set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${moddir})
+    if(ARG_BINDIR)
+      set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${ARG_BINDIR})
+    endif()
+
+    if(ARG_LIBDIR)
+      set_target_properties(${target} PROPERTIES ARCHIVE_OUTPUT_DIRECTORY ${ARG_LIBDIR})
+    endif()
+
+    if(moddir)
+      set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${moddir})
+    endif()
   endif()
 endfunction()

I have one other comment below.

Thanks,
-Chris

cmake/modules/AddLLVM.cmake

197 ↗

(On Diff #35980)

Two things here.

1. You need another empty string before ${ARGN} otherwise the first argument from ARGN will be treated as your multi-variable option list.

2. Maybe make the options more explicitly worded now that they are externally facing. I'd suggest BINARY_DIR and LIBRARY_DIR.

[john.brawn]

Made the suggested changes to the handling of unset arguments.

[beanz]

LGTM. Thanks!

-Chris

llvm/trunk/cmake/modules/AddLLVM.cmake

if(NOT LLVM_NO_DEAD_STRIP)
       endif()
     endif()
   endif()
 endfunction(add_link_opts)
 
 # Set each output directory according to ${CMAKE_CONFIGURATION_TYPES}.
 # Note: Don't set variables CMAKE_*_OUTPUT_DIRECTORY any more,
 # or a certain builder, for eaxample, msbuild.exe, would be confused.
-function(set_output_directory target bindir libdir)
-  # Do nothing if *_OUTPUT_INTDIR is empty.
-  if("${bindir}" STREQUAL "")
-    return()
-  endif()
+function(set_output_directory target)
+  cmake_parse_arguments(ARG "" "BINARY_DIR;LIBRARY_DIR;" "" ${ARGN})
 
-  # moddir -- corresponding to LIBRARY_OUTPUT_DIRECTORY.
+  # module_dir -- corresponding to LIBRARY_OUTPUT_DIRECTORY.
   # It affects output of add_library(MODULE).
   if(WIN32 OR CYGWIN)
     # DLL platform
-    set(moddir ${bindir})
+    set(module_dir ${ARG_BINARY_DIR})
   else()
-    set(moddir ${libdir})
+    set(module_dir ${ARG_LIBRARY_DIR})
   endif()
   if(NOT "${CMAKE_CFG_INTDIR}" STREQUAL ".")
     foreach(build_mode ${CMAKE_CONFIGURATION_TYPES})
       string(TOUPPER "${build_mode}" CONFIG_SUFFIX)
-      string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} bi ${bindir})
-      string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} li ${libdir})
-      string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} mi ${moddir})
+      if(ARG_BINARY_DIR)
+        string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} bi ${ARG_BINARY_DIR})
         set_target_properties(${target} PROPERTIES "RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${bi})
+      endif()
+      if(ARG_LIBRARY_DIR)
+        string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} li ${ARG_LIBRARY_DIR})
         set_target_properties(${target} PROPERTIES "ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${li})
+      endif()
+      if(module_dir)
+        string(REPLACE ${CMAKE_CFG_INTDIR} ${build_mode} mi ${module_dir})
         set_target_properties(${target} PROPERTIES "LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${mi})
+      endif()
     endforeach()
   else()
-    set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${bindir})
-    set_target_properties(${target} PROPERTIES ARCHIVE_OUTPUT_DIRECTORY ${libdir})
-    set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${moddir})
+    if(ARG_BINARY_DIR)
+      set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${ARG_BINARY_DIR})
+    endif()
+    if(ARG_LIBRARY_DIR)
+      set_target_properties(${target} PROPERTIES ARCHIVE_OUTPUT_DIRECTORY ${ARG_LIBRARY_DIR})
+    endif()
+    if(module_dir)
+      set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${module_dir})
+    endif()
   endif()
 endfunction()
 
 # If on Windows and building with MSVC, add the resource script containing the
 # VERSIONINFO data to the project.  This embeds version resource information
 # into the output .exe or .dll.
 # TODO: Enable for MinGW Windows builds too.
 #
else()
     add_library(${name} STATIC ${ALL_FILES})
   endif()
 
   if(DEFINED windows_resource_file)
     set_windows_version_resource_properties(${name} ${windows_resource_file})
     set(windows_resource_file ${windows_resource_file} PARENT_SCOPE)
   endif()
 
-  set_output_directory(${name} ${LLVM_RUNTIME_OUTPUT_INTDIR} ${LLVM_LIBRARY_OUTPUT_INTDIR})
+  set_output_directory(${name} BINARY_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR} LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
   # $<TARGET_OBJECTS> doesn't require compile flags.
   if(NOT obj_name)
     llvm_update_compile_flags(${name})
   endif()
   add_link_opts( ${name} )
   if(ARG_OUTPUT_NAME)
     set_target_properties(${name}
       PROPERTIES
if (LLVM_EXPORTED_SYMBOL_FILE)
     add_llvm_symbol_exports( ${name} ${LLVM_EXPORTED_SYMBOL_FILE} )
   endif(LLVM_EXPORTED_SYMBOL_FILE)
 
   if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
     set(USE_SHARED USE_SHARED)
   endif()
 
   set(EXCLUDE_FROM_ALL OFF)
-  set_output_directory(${name} ${LLVM_RUNTIME_OUTPUT_INTDIR} ${LLVM_LIBRARY_OUTPUT_INTDIR})
+  set_output_directory(${name} BINARY_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR} LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
   llvm_config( ${name} ${USE_SHARED} ${LLVM_LINK_COMPONENTS} )
   if( LLVM_COMMON_DEPENDS )
     add_dependencies( ${name} ${LLVM_COMMON_DEPENDS} )
   endif( LLVM_COMMON_DEPENDS )
 endmacro(add_llvm_executable name)
 
 function(export_executable_symbols target)
   if (NOT MSVC) # MSVC's linker doesn't support exporting all symbols.
function(add_unittest test_suite test_name)
   if (SUPPORTS_NO_VARIADIC_MACROS_FLAG)
     list(APPEND LLVM_COMPILE_FLAGS "-Wno-variadic-macros")
   endif ()
 
   set(LLVM_REQUIRES_RTTI OFF)
 
   add_llvm_executable(${test_name} ${ARGN})
   set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
-  set_output_directory(${test_name} ${outdir} ${outdir})
+  set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
   target_link_libraries(${test_name}
     gtest
     gtest_main
     LLVMSupport # gtest needs it for raw_ostream.
     )
 
   add_dependencies(${test_suite} ${test_name})
   get_target_property(test_suite_folder ${test_suite} FOLDER)