[patch] EXPORTED_SYMBOL_FILE using mingw and cmake

Hi,

this is my first post to this list, so please excuse if submitting a patch without previous discussion is considered bad form or anything similar. I encountered a bug in the CMake build while using MinGW (non-MSYS, non-CYGWIN) where the LTO_export fails with a “The syntax of the command is incorrect” error. This error was previously fixed for Windows in general using TO_NATIVE_PATH, however CMake has a known bug ( http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939 ) where TO_NATIVE_PATH does not replace slashes by backslashes when the MinGW Makefiles generator is used. The attached patch provides a workaround. Considering that the bug has been open since 2007 it is unlikely that kitware will fix this anytime soon.

Regards

Johannes S. Mueller-Roemer

AddLLVM.cmake.patch (699 Bytes)

@ Brad : This might be a bug worth taking a look at

Hi Johannes,

Thanks for brining this to my attention.

this is my first post to this list, so please excuse if submitting a patch
without previous discussion is considered bad form or anything similar.

It's fine to submit a patch without discussion first if it's minor
change like this one, but usually they should normally go to the
llvm-commits mailing list instead of llvmdev. I'll try to review it
anyway.

I
encountered a bug in the CMake build while using MinGW (non-MSYS,
non-CYGWIN) where the LTO_export fails with a “The syntax of the command is
incorrect” error. This error was previously fixed for Windows in general
using TO_NATIVE_PATH, however CMake has a known bug (
http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939 ) where
TO_NATIVE_PATH does not replace slashes by backslashes when the MinGW
Makefiles generator is used. The attached patch provides a workaround.
Considering that the bug has been open since 2007 it is unlikely that
kitware will fix this anytime soon.

Yes considering the age of that bug report the workaround is probably
the best thing to do.

At a glance this patch looks okay but I would change it slightly so

- In the comments have a link to the bug you are working around
- Quote "${export_file}" so we can handle paths with spaces in (I
realise the old code didn't do that, but that is probably a mistake).

Also could you use MINGW in your conditional like this so you only
workaround the bug when necessary instead of all Windows platforms?

if(MINGW)
  # mingw workaround, as TO_NATIVE_PATH is implemented incorrectly
  # See http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939
  string(REPLACE / \\ export_file_backslashes "${export_file}")
else()
  file(TO_NATIVE_PATH "${export_file}" export_file_backslashes)
endif()

I may have got this slightly wrong though as I'm not familiar with MinGW.

Thanks,

Changing it to only apply to the MinGW-Makefiles generator is maybe the safest method. Though you would have to check if MINGW is only set in the Mingw-Makefiles generator and not In the MSYS-Makefiles or Cygwin generators (should be in the docs).

Okay I guess the test would be something like this then

if (CMAKE_GENERATOR STREQUAL "MinGW Makefiles")
  # mingw workaround, as file(TO_NATIVE_PATH ...) is implemented incorrectly
  # See http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939
  string(REPLACE / \\ export_file_backslashes "${export_file}")
else()
  file(TO_NATIVE_PATH "${export_file}" export_file_backslashes)
endif()

I've written this into patch applied against trunk. Could you test
this (I don't use MinGW and don't have time to set it up) and confirm
it works? It would be great if you could test that the
file(TO_NATIVE_PATH ...) still gets used for the MSYS-Makefiles and
Cygwin generators.

It's also good to see some activity on the CMake bug report :slight_smile:

If it all works then I can commit on your behalf.

Thanks,
Dan

mingw_makefile_cmake_fix.patch (904 Bytes)

The new patch works fine, but considering the discussion on the CMake bug tracker, I'm would say that TO_NATIVE_PATH shouldn't be used at all.

I'm happy either way. A patch using string(REPLACE ...) instead for
all cases I think would be fine provided it has a comment saying
something like...

# Don't use file(TO_NATIVE_PATH ...) it doesn't work correctly when
generating MingW Makefiles
string(REPLACE / \\ export_file_backslashes "${export_file}")

so that someone doesn't try and start using file(TO_NATIVE_PATH ...)
again at some point. It would also be cleaner I guess.

Our exchange on the zlib thread reminded me, that we didn't finish the EXPORT_SYMBOL_FILE patch before my vacation.
Here is my newest version, which mentions the bug and always uses string(REPLACE)

AddLLVM.cmake.patch (964 Bytes)

LGTM. Do you have commit access?