This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Use backward slashes inside response files
ClosedPublic

Authored by grimar on Nov 9 2016, 2:26 AM.

Diff Detail

Event Timeline

grimar updated this revision to Diff 77324.Nov 9 2016, 2:26 AM
grimar retitled this revision from to [ELF] - Use backward slashes inside response files.
grimar updated this object.
grimar added reviewers: ruiu, rafael, davide.
grimar added subscribers: llvm-commits, grimar, evgeny777.
rafael added inline comments.Nov 9 2016, 5:31 AM
lib/Core/Reproduce.cpp
98

Exactly the same line is in CpioFile::append. Is it dead with this change?

grimar added inline comments.Nov 9 2016, 5:35 AM
lib/Core/Reproduce.cpp
98

I'll check, I did know about that one.

grimar added inline comments.Nov 9 2016, 7:20 AM
lib/Core/Reproduce.cpp
98

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.

ruiu added inline comments.Nov 9 2016, 1:36 PM
lib/Core/Reproduce.cpp
81

Please add a comment when you do something not obvious.

// Returned string is a slash-separated path even on Windows to avoid
// a mess with backslash-as-escape and backslash-as-path-separator.
test/ELF/reproduce-windows2.s
11

Don't use --to-stdout as it won't work on non-GNU systems.

grimar updated this revision to Diff 77461.Nov 10 2016, 2:42 AM
  • Addressed review comments
lib/Core/Reproduce.cpp
81

Done.

test/ELF/reproduce-windows2.s
11

I wonder how to make a test then.

I think we can not unpack cpio on windows as "can get over the path limit on windows" like mentioned in reproduce-error.s.
Since this test is about windows specific functionality, what about running it on windows only ?

rafael accepted this revision.Nov 10 2016, 6:01 AM
rafael edited edge metadata.

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

lib/Core/Reproduce.cpp
98

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?

test/lit.cfg
210

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?

This revision is now accepted and ready to land.Nov 10 2016, 6:01 AM
ruiu accepted this revision.Nov 10 2016, 12:52 PM
ruiu edited edge metadata.

LGTM

test/ELF/reproduce-windows2.s
11

Being on Windows doesn't mean that the installed cpio command is GNU's, but maybe it's a fairly reasonable assumption.

test/lit.cfg
210

Yup.

This revision was automatically updated to reflect the committed changes.