Page MenuHomePhabricator

[lit] Allow boolean expressions in REQUIRES and XFAIL and UNSUPPORTED
ClosedPublic

Authored by gparker42 on Mar 15 2016, 7:20 AM.

Details

Summary

This change allows the conditions in lit test files (REQUIRES, XFAIL, UNSUPPORTED) to be boolean expressions.

The change is otherwise intended to preserve the behavior of existing conditions in test files. I verified that these test suites run unchanged on OS X: llvm; llvm-lit; swift; clang (after a small patch). Additional test coverage is appreciated.

A condition line is now a comma-separated list of boolean expressions. Comma-separated expressions act as if each expression were on its own condition line:
For REQUIRES, if every expression is true then the test will run.
For UNSUPPORTED, if every expression is false then the test will run.
For XFAIL, if every expression is false then the test is expected to succeed. As a special case "XFAIL: *" expects the test to fail.

Examples:

Test is expected fail on 64-bit Apple simulators and pass everywhere else

XFAIL: x86_64 && apple && !macosx

Test is unsupported on Windows and non-Ubuntu Linux and supported everywhere else

UNSUPPORTED: linux && !ubuntu, system-windows

Syntax:

  • '&&', '||', '!', '(', ')'. 'true' is true. 'false' is false.
  • Each test feature is a true identifier.
  • Substrings of the target triple are true identifiers for UNSUPPORTED and XFAIL, but not for REQUIRES. This matches the current behavior. (FIXME should this be changed?)
  • All other identifiers are false.
  • Identifiers are [-+=._a-zA-Z0-9]* . This allows all feature variable names used in llvm and clang and swift and libc++ today.

Diff Detail

Repository
rL LLVM

Event Timeline

gparker42 updated this revision to Diff 50724.Mar 15 2016, 7:20 AM
gparker42 retitled this revision from to [lit] Allow boolean expressions in REQUIRES and XFAIL and UNSUPPORTED.
gparker42 updated this object.
gparker42 added a subscriber: llvm-commits.
gparker42 updated this revision to Diff 50730.Mar 15 2016, 7:33 AM

Added a comment about the new throw from Test.isExpectedToFail().
Fixed the run command for lit/tests/requires-missing.txt.

I'm unqualified to review the Python but one thing in the specification doesn't look right.

For XFAIL, if each expression is False then the test is expected to fail. As a special case "XFAIL: *" also expects the test to fail.

For XFAIL, if each expression is False then the test is expected to *pass*. As a special case "XFAIL: *" expects the test to fail.
The xfail-expr-* tests get this right.

gparker42 updated this object.Mar 15 2016, 5:13 PM

LGTM. I feel it might be exaggerated, though. :D

Random thoughts:

  • Is ',' actually as same as 'or' ?
  • Could we use not words but chars like "!" "&&" there?
utils/lit/lit/Test.py
5 ↗(On Diff #50730)

It is failing on py3.

from lit.BooleanExpression import BooleanExpression

, acts as or for UNSUPPORTED and XFAIL but as and for REQUIRES. Also I don't think we want to allow things like REQUIRES: (a, b) and c.

I chose the Python syntax for and/or/True/False on the assumption that we might want to allow more Python expressions in the future. (In fact my implementation started that way but it was overkill for my purposes.) Does anyone else have a preference here?

gparker42 updated this revision to Diff 50899.Mar 16 2016, 5:56 PM

Fix an import for Python 3.
Remove a duplicate copy of Test.isEarlyTest().

ddunbar edited edge metadata.Mar 24 2016, 9:06 PM

My comments:

  1. I am +1 on the change overall, I definitely think this will be nicer to have in place.
  1. It would be good to get documentation for this, in docs/CommandGuide/lit.rst. There isn't much description of the "ShTest" format yet, but we should definitely try and improve that.
  1. Since the primary clients of lit use "C-family" style boolean operators, I think my preference would be for using || / && / ! / true / false.
  1. I find it a bit unfortunate that the semantics for "," aren't the same between XFAIL/UNSUPPORTED and REQUIRES... I'm not sure if this is just so natural that it all works out, or if it would be better to simply require using ||/&& explicitly. Any thoughts here, did you do a survey of what is in use currently? Could we migrate to dropping ','?
  1. Would it be worth getting quoted string support? That might give a nice longer term path to having a clean syntax rather than the current identifier regex.

Thanks!

utils/lit/lit/BooleanExpression.py
45 ↗(On Diff #50899)

This can be lifted out to a global constant to save on the regex compile.

125 ↗(On Diff #50899)

I would write the tests for the basic parser inline here using the unit test style (see ShUtil.py), it is much more convenient and transparent than using lit for this kind of thing. The ShUtil tests themselves are run by the "tests/shell-parsing.py" test.

I see 92 test lines using ',' in llvm and clang and swift tests. About half of those of those are XFAILs in llvm, but REQUIRES and UNSUPPORTED are also represented.

Identifiers with '-' are everywhere. (Target triples, for one.) ':' and '/' may in fact be unused; I may have found them previously in tests that had text like "UNSUPPORTED:" in places that weren't FileCheck commands. '=' is used in Swift's tests but we can change that.

EricWF added a subscriber: EricWF.Mar 24 2016, 10:49 PM
gparker42 updated this revision to Diff 61628.Jun 22 2016, 5:51 PM
gparker42 updated this object.
gparker42 edited edge metadata.
  • Changed syntax to C-style ! || && instead of Python-style not or and.
  • Removed REQUIRES-ANY (superseding r 271468)
  • Removed identifier characters ":" and "/"
  • Added identifier characters "." and "+" (used by target triples and libc++)
  • Moved BooleanExpression tests to Python unittest.

LGTM, but can you add a section on the format to lit.pod? I know we don't have complete docs on the format, but if you could at least add section on the syntax it would be good.

I'll do that. docs/TestingGuide.rst needs to be updated too.

ddunbar accepted this revision.Sep 1 2016, 8:21 PM
ddunbar edited edge metadata.
This revision is now accepted and ready to land.Sep 1 2016, 8:21 PM

Looks like patch was not committed.

Greg, I assumed you had LLVM commit access and could land this?

Sorry, haven't had time to commit this yet.

gparker42 updated this revision to Diff 85079.Jan 19 2017, 5:54 PM

Belatedly picking this up again.

  • Resolved conflicts with custom keyword parser machinery in TestRunner.py
  • Updated docs/TestingGuide.rst
  • Loosened identifier syntax, in particular to allow more triple substrings like -linux-.
gparker42 updated this revision to Diff 85536.Jan 24 2017, 12:32 AM

Add an error message for REQUIRES-ANY: that suggests replacement by a REQUIRES: expression.

gparker42 edited the summary of this revision. (Show Details)Jan 24 2017, 12:32 AM
This revision was automatically updated to reflect the committed changes.

Could you please give me heads up before you commit this so I can be standing by reading to fix the libc++ bots?

Libc++ uses requires any and so if you commit this without allowing libc++ to update it will break all of its builders.

In an ideal world removing REQUIRES-ANY would be done as a separate change so as to give libc++ time to change.

Sorry, I thought libc++ had abandoned REQUIRES-ANY in favor of custom parsers. I just pushed the commit a few minutes ago. I'll revert it until we can coordinate better.

I'll reintroduce REQUIRES-ANY support so there is less disruption for libc++ developers.

gparker42 reopened this revision.Jan 24 2017, 1:55 AM
This revision is now accepted and ready to land.Jan 24 2017, 1:55 AM

I'll reintroduce REQUIRES-ANY support so there is less disruption for libc++ developers.

No need. I've already converted libc++ so it no longer uses REQUIRES-ANY. You should
be able to commit this patch know without breaking libc++.

gparker42 updated this revision to Diff 85547.Jan 24 2017, 1:58 AM

Reinstate support for REQUIRES-ANY:.

This revision was automatically updated to reflect the committed changes.