llvm dev - Jul 2014 - [LLVMdev] [patch] EXPORTED_SYMBOL_FILE using mingw and cmake

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).

--
Johannes S. Mueller-Roemer, MSc
Wiss. Mitarbeiter - Interactive Engineering Technologies (IET)

Fraunhofer-Institut für Graphische Datenverarbeitung IGD
Fraunhoferstr. 5  |  64283 Darmstadt  |  Germany
Tel +49 6151 155-606  |  Fax +49 6151 155-139
johannes.mueller-roemer at igd.fraunhofer.de  |  www.igd.fraunhofer.de


-----Original Message-----
From: Dan Liew [mailto:dan at su-root.co.uk] 
Sent: Monday, July 21, 2014 12:44
To: Mueller-Roemer, Johannes Sebastian
Cc: llvmdev at cs.uiuc.edu; Brad King
Subject: Re: [LLVMdev] [patch] EXPORTED_SYMBOL_FILE using mingw and cmake

@ 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,
--
Dan Liew
PhD Student - Imperial College London

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 :)

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

Thanks,
Dan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mingw_makefile_cmake_fix.patch
Type: text/x-patch
Size: 904 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140723/4521e368/attachment.bin>

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.
________________________________________
From: Dan Liew [dan at su-root.co.uk]
Sent: Wednesday, July 23, 2014 10:38 AM
To: Mueller-Roemer, Johannes Sebastian
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] [patch] EXPORTED_SYMBOL_FILE using mingw and cmake

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 :)

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

Thanks,
Dan