[PATCH] D26443: [ELF] - Use backward slashes inside response files

[Rafael Ávila de Espíndola via llvm-commits]

rafael accepted this revision.
rafael added a comment.
This revision is now accepted and ready to land.

LGTM by me with the suggested change, but please wait for Rui to comment.



================
Comment at: lib/Core/Reproduce.cpp:96
   path::append(Res, path::relative_path(Abs));
+  std::replace(Res.begin(), Res.end(), '\\', '/');
 
----------------
grimar wrote:
> grimar wrote:
> > rafael wrote:
> > > Exactly the same line is in CpioFile::append.  Is it dead with this change?
> > I'll check, I did know about that one.
> So no, it is not dead.
> 
> Current line is responsible for converting slashes in lines of response.txt file.
> CpioFile::append()'s line converts the archive filename.
> 
> I thought about placing them together in CpioFile::append:
> ```
>   // Use unix path separators so the cpio can be extracted on both unix and
>   // windows.
>   std::replace(Fullpath.begin(), Fullpath.end(), '\\', '/');
>   std::replace(Data.begin(), Data.end(), '\\', '/');
> ```
> 
> But that probably not good thing to do because in that way slashes will be converted for all responce file and not only for path lines, what can change the original invocation result line.
> Also it seems convinent that relativeToRoot() just always returns fixed path.
> 
Ah, I see it now that CpioFile::append uses path::append with might add a \. Even if that was changed to use '/' directly,  Basename might still have a \ in it.

So OK, we need the two calls, but can you factor it into a trivial helper function? Something like convertToUnixPathSeparator?



================
Comment at: test/lit.cfg:210
 
+# Running on Windows
+if platform.system() in ['Windows']:
----------------
llvm use the name system-windows.

This is basically assuming that the cpio on windows is gnu, which I think is fine. Rui, is that OK?


https://reviews.llvm.org/D26443