[lit] Remove %T

[kubamracek]

Summary

I've recently found a weird non-deterministic failure in the case-insensitive-include-ms.c, case-insensitive-include.c and case-insensitive-system-include.c testcases. The reason was that they actually share the same %T directory and when they're run in parallel, they can overwrite other test's data. Pretty terrible, right?

This patch changes lit so it generates a per-test temporary directory for %T and not a per-test-suite temporary dir. Another option would of course be to fix the above-mentioned tests, but do we really need to require test writers to remember that %T is not unique to the test?

[rnk]

Tests that have this bug simply did not read the documentation for %T: it is the parent directory of %t, which is a non-existent path unique for the test. Any test that fails this way should do RUN: mkdir %t and use %t instead of %T.

[george.karpenkov]

That would be inconsistent with TestingGuide.rst, if this change goes through, the documentation needs to be updated.
In any case, currently TestingGuide.rst directly contradicts lit.rst on the meaning of %T.

[george.karpenkov]

@rnk There are two documentation files. The lit.rst says it's unique, TestingGuide.rst says it's not.
So at least that should be fixed.
In any case, I disagree with your comment: what if the user still wants to have %t to signify a temporary file? Why an extra mkdir?

[rnk]

In D35396#809062, @george.karpenkov wrote:

@rnk There are two documentation files. The lit.rst says it's unique, TestingGuide.rst says it's not.
So at least that should be fixed.
In any case, I disagree with your comment: what if the user still wants to have %t to signify a temporary file? Why an extra mkdir?

Somebody has to do the mkdir, and it might as well be the test, since those execute in parallel. Right now lit makes Output directories for every suite, not for every test. Running mkdir for every llvm test seems like it would slow down startup.

[george.karpenkov]

@rnk Actually it would be quite easy to do the mkdir iff %T is present in the file, if performance is a concern.
I would argue that test runs should be isolated by the test runner, and test authors should not be trying to judge whether it's safe to share a directory.

In any case, inconsistent documentation should be updated one way or the other.

[rnk]

In D35396#809086, @george.karpenkov wrote:

@rnk Actually it would be quite easy to do the mkdir iff %T is present in the file, if performance is a concern.
I would argue that test runs should be isolated by the test runner, and test authors should not be trying to judge whether it's safe to share a directory.

In any case, inconsistent documentation should be updated one way or the other.

That sounds like a pretty good idea. :) FWIW, I am concerned about the performance. You can instrument lit to check out log it takes to make the Output directories.

[kubamracek]

I'm not suggesting to mkdir a new directory for each test. %T will point to a non-existent directory, just like %t points to a non-existent file. If the tests wants to use %T, it will have to mkdir it.

[rnk]

In D35396#809645, @kubamracek wrote:

I'm not suggesting to mkdir a new directory for each test. %T will point to a non-existent directory, just like %t points to a non-existent file. If the tests wants to use %T, it will have to mkdir it.

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

[kubamracek]

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

Oh. That's not what I meant then. I'll update the patch.

[kubamracek]

ping

[rnk]

In D35396#809760, @kubamracek wrote:

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

Oh. That's not what I meant then. I'll update the patch.

This doesn't seem like it was addressed? As is, surely this will regress lit startup time.

[kubamracek]

In D35396#822242, @rnk wrote:

In D35396#809760, @kubamracek wrote:

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

Oh. That's not what I meant then. I'll update the patch.

This doesn't seem like it was addressed? As is, surely this will regress lit startup time.

This was addressed, I changed the patch. dirname(tmpBase) is later being mkdir'd, but tmpDir isn't.

[rnk]

In D35396#822271, @kubamracek wrote:

This was addressed, I changed the patch. dirname(tmpBase) is later being mkdir'd, but tmpDir isn't.

Sounds good!

[kubamracek]

Cool. Some tests actually need fixing before this can land.

[MatzeB]

The fact that %T has the same values for all tests inside a directory is indeed terrible.

One thing to think about however: With this patch it seems %T and %t both point to a unique name that doesn't exist. So why have %T at all, you could just as well do mkdir %t and I think there already are a bunch of tests that do just that.

[kubamracek]

Works for me. @rnk, do you think that's better?

[MatzeB]

In D35396#822278, @MatzeB wrote:

The fact that %T has the same values for all tests inside a directory is indeed terrible.

One thing to think about however: With this patch it seems %T and %t both point to a unique name that doesn't exist. So why have %T at all, you could just as well do mkdir %t and I think there already are a bunch of tests that do just that.

Should have read the discussion before answering and realize you already discussed all that :)
(Guess there is just the fact that I lean towards the more aggressive solution that drops the concept of %T entirely. But that is easy to say if you're not the one having to fixup all the tests. In any way I'm fine with this approach too.

[rnk]

In D35396#822282, @kubamracek wrote:

Works for me. @rnk, do you think that's better?

+1 for removing %T. It's just a trap.

[george.karpenkov]

I also think that removing %T entirely in this case is a good idea.

[george.karpenkov]

@kubamracek Wouldn't you need now to change ALL the tests using %T? :P

[kubamracek]

Yes, of course, this change is not ready to land.

[kubamracek]

Patch to remove %T from compiler-rt: https://reviews.llvm.org/D36434

[kubamracek]

Clang: https://reviews.llvm.org/D36437

[kubamracek]

LLVM: https://reviews.llvm.org/D36495

[delcypher]

docs/TestingGuide.rst

459 ↗

(On Diff #109391)

What are we supposed to do to have a test written that way to work on Windows? There is a mkdir command but I'm not familiar enough with it to know if doing mkdir "%t" will work. It certainly doesn't take a -p argument.

[MatzeB]

docs/TestingGuide.rst

459 ↗

(On Diff #109391)

I have no experience with how we do things on windows, but I'm sure I've seen things like grep, sed etc. which aren't available on windows by default. I always assumed we expect people to install some posix compatible tools to run the tests on windows...

[george.karpenkov]

@delcypher I think LIT already does some magic to work with Windows, right? Many tests use mkdir, otherwise none of them would run.

[kubamracek]

@rnk will probably know. What is the story with mkdir -p %t on Windows?

[MatzeB]

And this LGTM.

[MatzeB]

And some heads up: Removing the tmpDir parameter from getDefaultSubstitutions will need some changes to the llvm test-suite lit integration which calls this function. But that's no reason to stop this patch, I'll update the test-suite when this is committed.

[delcypher]

In D35396#839504, @george.karpenkov wrote:

@delcypher I think LIT already does some magic to work with Windows, right? Many tests use mkdir, otherwise none of them would run.

The lit shell tests have two modes. Internal and external. External just invokes bash and the internal shell implements a shell command parser in python.

The executeScriptInternal() function in TestRunner.py handles running commands using the internal shell. This eventually calls _executeShCmd(). If you look at the implementation is has special support of a few shell built-ins (namely cd, echo, export and env). I don't see any special support for mkdir though. If the command doesn't start with . then lit.util.which() will be used to find the executable. So AFAICT mkdir will just run the mkdir command when using the internal shell.

The approach I've used in the past to handle different system tools is to add a substitution that expands to the command appropriate for the platform.

E.g.

https://github.com/symbooglix/symbooglix/blob/master/test_programs/lit.site.cfg#L116

[rnk]

In D35396#839505, @kubamracek wrote:

@rnk will probably know. What is the story with mkdir -p %t on Windows?

It works because LLVM requires Unix core utilities to be available. We have some advice on where to get them tucked away here: https://llvm.org/docs/GettingStartedVS.html#software It should probably be more prominent.

[dwiberg]

In D35396#840758, @rnk wrote:

In D35396#839505, @kubamracek wrote:

@rnk will probably know. What is the story with mkdir -p %t on Windows?

It works because LLVM requires Unix core utilities to be available. We have some advice on where to get them tucked away here: https://llvm.org/docs/GettingStartedVS.html#software It should probably be more prominent.

One thing to note here is that even if the GnuWin32 tools are available in PATH, if you type mkdir in a console window you seem to get the Windows version.

[rnk]

Is this ready?

[kubamracek]

Almost, I'm still working on removing "%T" throughout all the repos. Clang is clean, LLVM has 1 instance left, which I wasn't able to remove, and I asked the responsible person for help. There's also some downstream projects that need to be cleaned first (Swift).

[Lekensteyn]

While at it, could the documentation also be clarified whether %t refers to a prefix (or note that it is created in a subdirectory such that one can know for sure that it is cleaned up)? A lot of tests use something like %clang -o %t.o.

As for whether mkdir -p can be used or not, based on a look in utils/lit/, there does not seem to be a special case for that. Very few builtins (export, echo, cd) are handled. Perhaps it would be worth adding more builtins and/or document whether it is safe to use those? Looking at a Windows builder (http://lab.llvm.org:8011/buildslaves/windows7-buildbot) though, the Profile/gcc-flag-compatibility.c test does get executed properly (it uses rm -rf and mkdir -p ../..).

@rnk Are the GnuWin32 tools just needed for the test suite or is it also needed for the build process? (Linux user here, no idea how LLVM is usually developed on Windows)

[MatzeB]

In D35396#875872, @Lekensteyn wrote:

While at it, could the documentation also be clarified whether %t refers to a prefix (or note that it is created in a subdirectory such that one can know for sure that it is cleaned up)? A lot of tests use something like %clang -o %t.o.

%t is just a name/string that is based on the tests name and is therefore guaranteed to be unique for each test. There is no automatic cleanup, in fact I believe you don't want automatic cleanup as you want to inspect the intermediate results in case of errors.
However I believe the %t path is always in a subdirectory called Output so if you wanted to cleanup intermediate results between test-suite runs you could do something like find build/test -d -name "Output" -exec rm -rf {} ';'

As for whether mkdir -p can be used or not, based on a look in utils/lit/, there does not seem to be a special case for that. Very few builtins (export, echo, cd) are handled. Perhaps it would be worth adding more builtins and/or document whether it is safe to use those? Looking at a Windows builder (http://lab.llvm.org:8011/buildslaves/windows7-buildbot) though, the Profile/gcc-flag-compatibility.c test does get executed properly (it uses rm -rf and mkdir -p ../..).

The tests assume this is possible even on windows. So the expectation is that whoever runs the tests has those GnuWin32 tools installed.

@rnk Are the GnuWin32 tools just needed for the test suite or is it also needed for the build process? (Linux user here, no idea how LLVM is usually developed on Windows)

docs/GettingStartedVS.rst sounds like GnuWin32 is only needed for the tests.

[Lekensteyn]

Hi, what is the status with this patch? It was referred to by https://reviews.llvm.org/D36495 (which is committed), and there still seem to be a lot of users of %T.

[kubamracek]

In D35396#1132499, @Lekensteyn wrote:

Hi, what is the status with this patch? It was referred to by https://reviews.llvm.org/D36495 (which is committed), and there still seem to be a lot of users of %T.

Yes, that's why this one didn't land yet. We need to get rid of all the %T users first.

[Lekensteyn]

In D35396#1132500, @kubamracek wrote:

In D35396#1132499, @Lekensteyn wrote:

Hi, what is the status with this patch? It was referred to by https://reviews.llvm.org/D36495 (which is committed), and there still seem to be a lot of users of %T.

Yes, that's why this one didn't land yet. We need to get rid of all the %T users first.

Perhaps you could already adjust the documentation and mark %T as deprecated. I have honestly forgotten about this patch and used %T again in some new tests because it was documented (without any caveats).

[kubamracek]

Good idea. https://reviews.llvm.org/D48189.

utils/lit/lit/TestRunner.py

def parseIntegratedTestScriptCommands(source_path, keywords):
     finally:
         f.close()
 
 def getTempPaths(test):
     """Get the temporary location, this is always relative to the test suite
     root, not test source root."""
     execpath = test.getExecPath()
     execdir,execbase = os.path.split(execpath)
-    tmpDir = os.path.join(execdir, 'Output')
+    tmpDir = os.path.join(execdir, 'Output', execbase + ".tmpdir")
     tmpBase = os.path.join(tmpDir, execbase)
     return tmpDir, tmpBase
 
 def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
     sourcepath = test.getSourcePath()
     sourcedir = os.path.dirname(sourcepath)
 
     # Normalize slashes, if requested.