This is an archive of the discontinued LLVM Phabricator instance.

lit: Improve %: normalization.
ClosedPublic

Authored by pcc on Oct 17 2017, 4:14 PM.

Details

Summary

The new scheme should match the normalization of embedded paths in
linkrepro tar files.

Event Timeline

pcc created this revision.Oct 17 2017, 4:14 PM
zturner added inline comments.Oct 17 2017, 4:24 PM
llvm/utils/lit/lit/TestRunner.py
870–879

I think the %: substitutions are used outside of linkrepro tar files, so it seems odd to change the semantics of ALL substitutions just for the purposes of a small number of tests.

We already have %: (remove :), and %/ (convert \ to /). Maybe adding %:/ (remove : *and* convert \ to /) is a better choice? This way if for some reason someone still needs to use the existing semantics of %:, they can get it.

ruiu added inline comments.Oct 17 2017, 4:24 PM
llvm/utils/lit/lit/TestRunner.py
830

This returns /foo for C:\foo, no?

ruiu edited edge metadata.Oct 17 2017, 4:25 PM

I introduced %:X lit variables for linkrepro tests, and I doubt we use them in other tests.

pcc added inline comments.Oct 17 2017, 4:28 PM
llvm/utils/lit/lit/TestRunner.py
830

I think it returns C/foo. The replace will return C:/foo, and then the sub will match the C: and replace it with C.

870–879

I couldn't find any though (did a grep for %: over the monorepo).

zturner edited edge metadata.Oct 17 2017, 4:32 PM
In D39023#900213, @ruiu wrote:

I introduced %:X lit variables for linkrepro tests, and I doubt we use them in other tests.

In that case, I would suggest a couple of different alternatives:

  1. If it's really not usable outside of LLD, add the substitutions to LLD's lit.cfg.py instead of to LLVM's TestRunner.py. A person writing a test for clang doesn't care about linkrepro TAR files and shouldn't have to understand how this comment applies to them. Worse, lit is downloadable as a public package from various package managesr, and these people using lit are not even doing anything related to LLVM (or even compilers) at all. It doesn't make sense for them to read a comment about linkrepro tar files. If it's in the core lit infrastructure, it has to be independent of how we (LLVM/LLD/Clang) decide to use it.
  1. Re-write the comment to explain what it does, but don't mention linkrepro or tar files. For the same reason as above. (Basically: Just delete the second sentence).

I did a grep of the code, and you're right that these substitutions are not used outside of lld. For that reason, number 1 seems more "correct", but number 2 is less work to change.

pcc updated this revision to Diff 119400.Oct 17 2017, 4:37 PM
  • Remove the comment about linkrepro
pcc added a comment.Oct 17 2017, 4:39 PM

Okay, I did 2.

zturner accepted this revision.Oct 17 2017, 4:43 PM
This revision is now accepted and ready to land.Oct 17 2017, 4:43 PM
This revision was automatically updated to reflect the committed changes.