[PATCH] EXPORTED_SYMBOL_FILE using mingw and cmake

[Chris Bieneman]

> On Oct 22, 2014, at 11:59 PM, Mueller-Roemer, Johannes Sebastian <Johannes.Sebastian.Mueller-Roemer at igd.fraunhofer.de> wrote:
> 
> It has to be moved there beause Cygwin uses forward slashes (and cat), as it is a full-fledged posix environment.
> Assuming WIN32 unconditionally should be fine, as by default the whole block uses type instead of cat. Which is Windows only.

I disagree. I think the fact that the whole block uses type instead of cat is just another example that this block needs to be cleaned up. You’ve also missed my point about export_file_backslashes. You are ONLY setting it on one branch of the if statement. This will not work.

I think a more reasonable diff would be:

diff --git a/cmake/modules/AddLLVM.cmake b/cmake/modules/AddLLVM.cmake
index 81b21fb..9ebeca4 100644
--- a/cmake/modules/AddLLVM.cmake
+++ b/cmake/modules/AddLLVM.cmake
@@ -85,17 +85,20 @@ function(add_llvm_symbol_exports target_name export_file)
   else()
     set(native_export_file "${target_name}.def")
 
-    set(CAT "type")
-    if(CYGWIN)
-      set(CAT "cat")
+    set(CAT "cat")
+    set(export_file_nativeslashes ${export_file}) 
+    if(WIN32 AND NOT CYGWIN)
+      set(CAT "type")
+      # Convert ${export_file} to native format (backslashes) for "type"
+      # Does not use file(TO_NATIVE_PATH) as it doesn't create a native
+      # path but a build-system specific format (see CMake bug
+      # http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939 )
+      string(REPLACE / \\ export_file_nativeslashes ${export_file})
     endif()
 
-    # Using ${export_file} in add_custom_command directly confuses cmd.exe.
-    file(TO_NATIVE_PATH ${export_file} export_file_backslashes)
-
     add_custom_command(OUTPUT ${native_export_file}
       COMMAND ${CMAKE_COMMAND} -E echo "EXPORTS" > ${native_export_file}
-      COMMAND ${CAT} ${export_file_backslashes} >> ${native_export_file}
+      COMMAND ${CAT} ${export_file_nativeslashes} >> ${native_export_file}
       DEPENDS ${export_file}
       VERBATIM
       COMMENT "Creating export file for ${target_name}")


This diff I believe should both handle the case you’re trying to handle and ensure that in the future if we use a linker that doesn’t require a link version script on Linux we won’t inadvertently be using backslashes in paths. I’ve also re-named the export_file_backslashes variable to export_file_nativeslashes because the reality is that this variable may have forward or back slashes based on which OS & toolchain is in use.

-Chris

> ________________________________________
> From: Chris Bieneman [beanz at apple.com]
> Sent: Wednesday, October 22, 2014 5:57 PM
> To: Mueller-Roemer, Johannes Sebastian
> Cc: beanz at apple.com; llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] EXPORTED_SYMBOL_FILE using mingw and cmake
> 
> In general I don't think I should be the one to review this patch because it seems to be Windows specific, and I don't really do dev on Windows. However, since you ping'd me, see my comment below.
> 
> REPOSITORY
>  rL LLVM
> 
> ================
> Comment at: cmake/modules/AddLLVM.cmake:96
> @@ -91,1 +95,3 @@
> +      # http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939 )
> +      string(REPLACE / \\ export_file_backslashes ${export_file})
>     endif()
> ----------------
> By moving the definition of export_file_backslashes into this else clause it won't be defined on Cygwin, so I don't think this will build.
> 
> I also think this should probably actually be in a separate if block for "if (WIN32 and not CYGWIN)". My reasoning for that (which may be wrong, so take it with some salt) is you're in a block now that is basically "if (not Darwin and not LLVM_HAVE_LINK_VERSION_SCRIPT)". In practice that seems to mean WIN32, but I don't think that is a hard requirement. In particular since this is really related to linker features it is plausible that in the near future lld may not require a link version script, and you could use it on any platform.
> 
> http://reviews.llvm.org/D5476
> 
>