llvm dev - Jul 2014 - [LLVMdev] [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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140617/34f7bf8c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AddLLVM.cmake.patch
Type: application/octet-stream
Size: 699 bytes
Desc: AddLLVM.cmake.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140617/34f7bf8c/attachment.obj>

@ 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

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