[RFC] Remove Support for %T from llvm-lit

The %T substitution in llvm-lit returns a temporary folder common to the directory that the test is in. Because it is not unique to the individual test, %T can lead to races if not used carefully. Because of this it has been deprecated for about seven years (https://github.com/llvm/llvm-project/commit/a2e0c2462a5ed4ddabab03dfddb97efc54682bde). There were very few tests in tree still using this functionality. I have recently cleaned up the remaining uses in the tree and am looking to remove support for %T from lit so new tests do not end up using the feature. I wanted to solicit feedback from the community on the best way to do this to make this reasonably easy for downstreams.

I think there are three feasible options with varying degrees of friction for downstreams:

  1. Add a flag or config option to disable %T substitutions within the monorepo. We would then enable this flag within the monorepo to prevent usage there. This has the upside of being zero friction for downstreams as it is opt-in. This doesn’t have a good path for fully deprecating %T though and adds the maintenance burden of maintaining a flag.

  2. We could disable %T by default but add a flag/config option to restore the previous behavior that we eventually plan to fully remove. This would mean downstreams only need to change a line or two of configs/build system logic to get the previous behavior while still allowing for a full deprecation. This is slightly more friction and keeps some maintenance in lit itself, but has some advantages. It ensures downstreams are aware of the issue because they have to opt in and it scopes the upstream maintenance burden to a specific period of time.

  3. We rip out all support for %T and require downstreams that still want to use the feature to manually add the substitution (most likely by subclassing ShTest if they do not already have a custom test format). This has the advantage that it is zero maintenance today and does not require any extended deprecation period. It has the downside of being more complicated for downstreams to implement than the previous two, although is still relatively uncomplicated.

There is one upstream consumer of %T in libc++, and we are planning on just adding a custom substitution within libc++’s custom test format, as described in option 3.

I do not work downstream much so I am unsure what people prefer. I think option three is the most attractive as it is not too much work for downstreams and keeps upstream maintenance to a minimum.

8 Likes

I’ve just searched the downstream PlayStation toolchain codebase for instances of %T and as far as I can see, there’s only a single test using %T in our downstream-specific tests. It’s possible I missed some, but either way it’s likely to be rare and it would be trivial to update the test to work without it, so I vote for option 3.

More generally, I think the migration path is straightforward enough in that downstream users could easily implement the substitution themselves by simply reinstating the code that the change removes, if they aren’t able to immediately update their tests. We don’t generally try to completely avoid breaking downstream codebases, so that reinforces my +1 to option 3.

5 Likes

I also think option (3) should be fine. I looked into our downstream diff and we don’t have a significant number of occurrences of the substitution.

However, I think we should consider whether we can explicitly error-out and provide a helpful diagnostic if lit finds %T in a command-line it is executing. We could leave that during one or two releases as a cheap way to ensure that people who were relying on %T now reliably get an error message. Otherwise, I can imagine the removal causing some confusing issues for downstreams, when a clear diagnostic would have made it into a 15 minutes problem to solve.

Good shout. Strong +1 to this suggestion.

Alternatively/in addition, perhaps we should error in lit for all substitutions that aren’t actually recognised? Unless we already do that?

Option 3 gets a +1 from me.

Option 1 and 2 would incur higher long-term costs from upstream than “potential” downstream projects for something that is rare and easy to change.

I don’t think we can do it for substitutions in the general case because we can’t tell whether arbitrary tokens are intended to be substitutions and the substitution is missing, or if the tokens are not intended to be substituted.

This only works for eg %T since we know that it is a substitution today and we are removing it.

Okay thanks.

We discussed this in the LLVM area team meeting and there is consensus for option 3 and to just fully remove %T

1 Like