[cmake-developers] install bug on windows
Bill Hoffman
bill.hoffman at kitware.com
Mon Sep 20 16:20:43 EDT 2010
On 9/20/2010 1:39 PM, Eric Noulard wrote:
> 2010/9/20 Eric Noulard<eric.noulard at gmail.com>:
>> 2010/9/20 Bill Hoffman<bill.hoffman at kitware.com>:
>>>
>>> Eric,
>>>
>>> This commit is breaking windows installs:
>>>
>>> commit 6a521f8604ee4e6a757109e731a36fdc5575f6c8
>>> Author: Eric NOULARD<eric.noulard at gmail.com>
>>> Date: Mon Aug 23 17:38:33 2010 +0200
>>>
>>> CPack Enable better handling of absolute installed files
>>>
>>> The idea of the patch is to let the install generator define
>>> CPACK_ABSOLUTE_INSTALL_FILES then when CMake is installing
>>> project he will concatenate the list of files and give
>>> it to specific CPack Generator by defining CPACK_ABSOLUTE_INSTALL_FILES
>>> to be the list of ALL files that were installed using absolute
>>> destination.
>>> An example of use has been applied to RPM generator which now
>>> tries to automatically build a relocatable package.
>>>
>
> [...]
>
>>> Why do you convert the / to an output path?
>>
>> I do not remember ... :-(
>>
>> I'm looking into this just now, give half an hour and I'll come
>> with some answer.
>
> Now I know: this is a plain mistake :-).
>
>>> 6a521f86 (Eric NOULARD 2010-08-23 17:38:33 +0200 71) os<< dest<<
>>> cmSystemTools::ConvertToOutputPath("/");
>>> 6a521f86 (Eric NOULARD 2010-08-23 17:38:33 +0200 72) if (rename&&
>>> *rename)
>>>
>>> All paths in the cmake_install file should be cmake paths as cmake is going
>>> to consume them.
>>
>> Off course, you are right.
>
> Considering this is plain mistake, the fix is obvious
>
> os<< dest<< "/";
>
> corresponding patch is attached.
>
> Would you like me to apply it to next or would you
> prefer to do it yourself?
>
>>> All that said, I am not sure why no test failed from this ????
>>
>> May be because there is no test with absolutely installed file
>> in the test suite?
>>
>> I'll have a look too.
>
> OK I think I know.
> The provided example from Micha Renner is a little awkward
> it uses "unix-like" absolute install path "/usr/local"
> on a Windows hosts :-)
>
> May be it's supposed to work but I wonder if
> it's a common usage pattern.
>
> The bare reality is twofold:
>
> a) I do not use windows platform myself so I rely
> on dashboard for regression.
>
> b) I did not add a [cross platform] test for this change
>
> I shall try to write one test for this, but this may not be easy
> to write a cross-platform one because I'll have to find a "writable"
> "absolute destination" for each platform.
>
> On Unix platforms "/tmp" should be ok
> On WIndows I really don't know.
>
> What do you think?
Seems to be covered:
(but I am guessing the install is not actually being called in that case...)
http://www.cdash.org/CDash/viewCoverageFile.php?buildid=726294&fileid=11009794
TF 63 if (cmSystemTools::FileIsFullPath(dest.c_str()))
54 | 64 {
55 | 65 os << "list(APPEND
CPACK_ABSOLUTE_DESTINATION_FILES\n";
56 | 66 os << indent << " \"";
57 | 67
for(std::vector<std::string>::const_iterator fi = files.begin
58 | TF 68 fi != files.end(); ++fi)
59 | 69 {
60 | -->F 70 if (fi!=files.begin()) os << ";";
61 | 71 os << dest <<
cmSystemTools::ConvertToOutputPath("/
62 | -->F 72a if (
63 | -->t 72b rename &&
64 | -->f 72c *rename)
65 | 73 {
66 | 74 os << rename;
67 | 75 }
68 | ...
69 | 81 os << "\")\n";
70 | 82 }
BTW, this type of code is not allowed in CMake:
if (fi!=files.begin()) os << ";";
Needs to be:
if((fi!=files.begin())
{
os << ";";
}
More information about the cmake-developers
mailing list