Page MenuHomePhabricator

Added REQUIRES to TestingGuide documentation
ClosedPublic

Authored by Prazek on Jul 8 2016, 2:28 PM.

Event Timeline

Prazek updated this revision to Diff 63318.Jul 8 2016, 2:28 PM
Prazek retitled this revision from to Added REQUIRES to FileCheck documentation.
Prazek updated this object.
Prazek added reviewers: alexfh, wolfgangp, rengolin.
Prazek added a subscriber: llvm-commits.

I have found the features list that you were looking for :D

REQUIRES is a 'lit' feature, not a FileCheck feature.
A better place to describe it would be docs/TestingGuide.rst, which already talks about RUN and XFAIL.

Prazek updated this revision to Diff 63328.Jul 8 2016, 3:11 PM

review fix

Prazek updated this revision to Diff 63329.Jul 8 2016, 3:13 PM

changed commit name

Prazek retitled this revision from Added REQUIRES to FileCheck documentation to Added REQUIRES to TestingGuide documentation.Jul 8 2016, 3:14 PM

I think you can also use triple components in REQUIRES, which makes me wonder about 'x86_64-linux'. Is that something lit supports separately from the triple check?

docs/TestingGuide.rst
399 ↗(On Diff #63353)

I think you need a colon after REQUIRES.

401 ↗(On Diff #63353)

coma -> comma

402 ↗(On Diff #63353)

I would rather see the two forms contrasted more directly, maybe like this:

REQUIRES means all listed requirements must be satisfied; REQUIRES-ANY means at least one must be satisfied.

429 ↗(On Diff #63353)

Please sort the list alphabetically. If you prefer to keep the "not" versions with the positive versions, that's fine.

Just curious, what is 'xar'?

Prazek marked 4 inline comments as done.Jul 8 2016, 3:40 PM
Prazek added inline comments.
docs/TestingGuide.rst
429 ↗(On Diff #63353)

no idea, but it comes from test/lit.cfg

if config.have_libxar:

config.available_features.add('xar')

so I guess libxar something

Prazek updated this revision to Diff 63338.Jul 8 2016, 3:47 PM
Prazek marked an inline comment as done.

post review

probinson accepted this revision.Jul 8 2016, 3:54 PM
probinson added a reviewer: probinson.

LGTM

This revision is now accepted and ready to land.Jul 8 2016, 3:54 PM

I'd mention where this list is coming from, or how to regenerate it. I fear this is going to be out-of-sync to quickly.

Prazek updated this revision to Diff 63353.Jul 8 2016, 4:16 PM
Prazek marked an inline comment as done.
Prazek edited edge metadata.

LAst review

This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Jul 8 2016, 5:16 PM
MatzeB added inline comments.
llvm/trunk/docs/TestingGuide.rst
406–434

Should we really list all of them here? I see this list getting out of date quickly as it is not obvious to people changing lit.cfg that this list exists here. I'd recommend to only refer to lit.cfg and maybe mention 1 or 2 examples...

mehdi_amini added inline comments.Jul 8 2016, 5:18 PM
llvm/trunk/docs/TestingGuide.rst
406–434

Agree.

Prazek added inline comments.Jul 8 2016, 5:29 PM
llvm/trunk/docs/TestingGuide.rst
406–434

I think having list somewhere is usefull. It wasn't obvious for me where I can find the code that defines the features - and all features are splitted between different files. The argument that it get outdated fast ins invalid - just keep track of it in reviews (I am willing to be added as a subscriber to every patch that touches any lit.cfg to check it)
or find a way to generate it from code.

The list is useful. e.g. I can see that the list doesn't mention anything similar to LLVM_ENABLE_STATS that I was looking for, so I know that I should probably go different way.

mehdi_amini added inline comments.Jul 8 2016, 5:37 PM
llvm/trunk/docs/TestingGuide.rst
406–434

The argument that it get outdated fast ins invalid - just keep track of it in reviews (I am willing to be added as a subscriber to every patch that touches any lit.cfg to check it)

Sorry but you can't just say that the argument "is invalid". Experience shows it is.

or find a way to generate it from code.

Sure, in the meantime I think we should remove it.

A list is definitely useful. It's a reasonable point that the list evolves over time and a manually maintained list is a headache.
Maybe add a command-line option to 'lit' that will dump the REQUIRES features. Then document that option in TestingGuide.rst, instead of having the list.

ok, I guess you are right.

here are the fixes.
http://reviews.llvm.org/D22245