Bug 75860 - CMake xmlto invocations are not compatible with MSYS
Summary: CMake xmlto invocations are not compatible with MSYS
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-07 00:19 UTC by LRN
Modified: 2014-03-22 20:10 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Make documentation generating MSYS2-compatible (1.22 KB, patch)
2014-03-07 00:20 UTC, LRN
Details | Splinter Review
Make documentation generating MSYS/MSYS2-compatible (1.86 KB, patch)
2014-03-17 15:09 UTC, LRN
Details | Splinter Review

Description LRN 2014-03-07 00:19:06 UTC
xmlto is a shell script, it needs to be fed MSYSsy filenames. CMake, if built as a pure W32 application (which it should be), will never provide those without special help.
Comment 1 LRN 2014-03-07 00:20:03 UTC
Created attachment 95269 [details] [review]
Make documentation generating MSYS2-compatible

This patch adds a cygpath invocation for filename conversion (autotools do
that automatically, for CMake you have to spell it out). Cygwpath is available
in MSYS2 (and Cygwin, obviously). It may be absent in MSYS, but i don't care.
Comment 2 Simon McVittie 2014-03-13 12:13:26 UTC
dbus/doc is a trap (Bug #67034), reassigning to dbus/core.
Comment 3 Simon McVittie 2014-03-13 12:18:52 UTC
I have no idea whether this patch is good. Ralf?
Comment 4 Ralf Habacker 2014-03-17 11:10:21 UTC
Comment on attachment 95269 [details] [review]
Make documentation generating MSYS2-compatible

Review of attachment 95269 [details] [review]:
-----------------------------------------------------------------

::: cmake/doc/CMakeLists.txt
@@ +72,5 @@
>  	  endif ()
>  	  if (XMLTO_EXECUTABLE)
> +		    if(MSYS)
> +		      execute_process(
> +			COMMAND cygpath ${_infile}

http://stackoverflow.com/questions/12015348/msys-path-conversion-or-cygpath-for-msys mentions that cygpath is not a core msys component and that this patch only works if cygpath is present. 
The website mentiones an alternative way without the need of a 3rd party tools 

sh -c 'cd /c/some/path; pwd -W'
Comment 5 LRN 2014-03-17 12:02:47 UTC
Repeating my original comment to the patch i've attached (after s/Cygwpath/Cygpath/):
> Cygpath is available in MSYS2 (and Cygwin, obviously).
> It may be absent in MSYS, but i don't care.

Worst case scenario: MSYS users will have to create a /bin/cygpath script that will run whatever they need to convert paths.

Best case scenario: Someone who cares about MSYS will come forward and provide a patch for your cmake scripts.
Comment 6 Ralf Habacker 2014-03-17 12:56:52 UTC
(In reply to comment #5)
> Repeating my original comment to the patch i've attached (after
> s/Cygwpath/Cygpath/):
I assume that the MSYS cmake variable is 1 on MSYS *AND* MSYS2 environments ?
Then this patch claims to support msys and msys2. 

To solve this problem you should either 
1. check if cygpath is available (which limits supported environment to MSYS2 and MSYS with cygpath) or 
2. limit the detection to MSYS2 only (which implies to have cygpath) or
3. use a cygpath emulation inside cmake (which has been mentioned before)
Comment 7 LRN 2014-03-17 13:05:25 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Repeating my original comment to the patch i've attached (after
> > s/Cygwpath/Cygpath/):
> I assume that the MSYS cmake variable is 1 on MSYS *AND* MSYS2 environments ?
That is a good question.
I have no answer, i don't know how cmake detects MSYS.
That said, the correct way to do so is to check the presence of the MSYSTEM environment variable. It is set in all MSYSes.

> 
> To solve this problem you should either 
> 1. check if cygpath is available (which limits supported environment to
> MSYS2 and MSYS with cygpath) or 
Presence of cygpath, in conjunction with the MSYS variable being set, would be a good indicator. I don't know how to check for programs in cmake scripts though.

> 2. limit the detection to MSYS2 only (which implies to have cygpath) or
I can't, off the top of my head, come up with a way to make a distinction between MSYS and MSYS2.

> 3. use a cygpath emulation inside cmake (which has been mentioned before)
I know little of cmake. What emulation are you talking about?
Comment 8 Ralf Habacker 2014-03-17 13:56:44 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Repeating my original comment to the patch i've attached (after
> > > s/Cygwpath/Cygpath/):
> > I assume that the MSYS cmake variable is 1 on MSYS *AND* MSYS2 environments ?
> That is a good question.
> I have no answer, i don't know how cmake detects MSYS.
> That said, the correct way to do so is to check the presence of the MSYSTEM
> environment variable. It is set in all MSYSes.
> 
> > 
> > To solve this problem you should either 
> > 1. check if cygpath is available (which limits supported environment to
> > MSYS2 and MSYS with cygpath) or 
> Presence of cygpath, in conjunction with the MSYS variable being set, would
> be a good indicator. I don't know how to check for programs in cmake scripts
> though.
http://www.cmake.org/cmake/help/v2.8.11/cmake.html#command:find_program
 
if(MSYS)
   find_program(CYPPATH_EXECUTABLE cygpath)
   if(CYPPATH_EXECUTABLE)
       ...
   endif()
endif()
> 
> > 2. limit the detection to MSYS2 only (which implies to have cygpath) or
> I can't, off the top of my head, come up with a way to make a distinction
> between MSYS and MSYS2.
> 
> > 3. use a cygpath emulation inside cmake (which has been mentioned before)
> I know little of cmake. What emulation are you talking about?

On the msys shell the following command returns the requested native path

sh -c 'cd /c/some/path; pwd -W'

This need to be run by cmake, which may be implemented as shown below:

if(MSYS)
    execute_process(COMMAND dirname ${_infile}  OUTPUT_VARIABLE _path)
    string(STRIP ${_path} _path)
    execute_process(COMMAND sh -c "cd ${_path}; pwd -W" OUTPUT_VARIABLE _path)
    string(STRIP ${_path} _path)
    set(_infile "${_path}/${_name}")
endif()

BTW: The string(STRIP ...) lines removes trailing lf from the result
Comment 9 LRN 2014-03-17 15:09:45 UTC
Created attachment 95942 [details] [review]
Make documentation generating MSYS/MSYS2-compatible

Something like this then?
Comment 10 Ralf Habacker 2014-03-18 06:49:14 UTC
Comment on attachment 95942 [details] [review]
Make documentation generating MSYS/MSYS2-compatible

Review of attachment 95942 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 11 Ralf Habacker 2014-03-22 20:10:33 UTC
committed to master


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.