This is an archive of the discontinued LLVM Phabricator instance.

[lit] Remove dead code not referenced in the LLVM SVN repo.
ClosedPublic

Authored by dlj on Jun 28 2017, 5:33 PM.

Details

Summary

This change removes the intermediate 'FileBasedTest' format from lit. This
format is only ever used by the ShTest format, so the logic can be moved into
ShTest directly.

In order to better clarify what the TestFormat subclasses do, I fleshed out the
TestFormat base class with Python's notion of abstract methods, using
@abc.abstractmethod. This gives a convenient way to document the expected
interface, without the risk of instantiating an abstract class (that's what
ABCMeta does -- it raises an exception if you try to instantiate a class which
has abstract methods, but not if you instantiate a subclass that implements
them).

Event Timeline

dlj created this revision.Jun 28 2017, 5:33 PM
modocache accepted this revision.Jun 28 2017, 5:43 PM

Awesome, thank you!

utils/lit/lit/formats/base.py
21

I greatly appreciate the documentation here. Thanks!

utils/lit/lit/formats/shtest.py
24

Question: The documentation for lit spells it with a lowercase 'l': http://llvm.org/docs/CommandGuide/lit.html. http://llvm.org/docs/TestingGuide.html, on the other hand, uses both capital and lowercase in some places. Which is correct?

I had always thought that lowercase "lit" was the proper name. If you agree, then this line and the "Args" section below should be updated.

This revision is now accepted and ready to land.Jun 28 2017, 5:43 PM
dlj updated this revision to Diff 104562.Jun 28 2017, 6:00 PM
dlj marked 2 inline comments as done.

Changed spelling: Lit -> lit.

utils/lit/lit/formats/base.py
21

Glad it helps!

utils/lit/lit/formats/shtest.py
24

I think I agree more with your logic... this was probably a typo, after typing LitConfig so many times.

This revision was automatically updated to reflect the committed changes.

Hi. There are some failures on LLVM test-suite[1] after this code was merged.

"FileBasedTest" is being imported at litsupport/test.py:10 (currently rev. 306655) and it is causing lnt to fail abruptly when running "lnt runtest test-suite".

It is not possible to observe failures on lnt public bots as they are running "lnt runtest nt", not "lnt runtest test-suite".

May I suggest to revert the commit or adjust so it doesn't break the existing test ?

[1] http://llvm.org/svn/llvm-project/test-suite/trunk/

We're seeing more issues (I think they are unrelated to the FileBasedTest issue mentioned before).

[1] is one example of the failure, the error is:

lit.py: /Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg', traceback: Traceback (most recent call last):
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg", line 52, in <module>
    configuration.configure()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 129, in configure
    self.configure_triple()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1073, in configure_triple
    self.use_deployment = not self.use_target and self.can_use_deployment()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1059, in can_use_deployment
    if not self.target_info.is_host_macosx():
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/target_info.py", line 76, in is_host_macosx
    name = lit.util.capture(['sw_vers', '-productName']).strip()
AttributeError: 'module' object has no attribute 'capture'

Any ideas on how to fix it?

[1]: http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/32959/consoleFull#-42777206a1ca8a51-895e-46c6-af87-ce24fa4cd561

dlj added a comment.Jun 29 2017, 4:00 PM

We're seeing more issues (I think they are unrelated to the FileBasedTest issue mentioned before).

[1] is one example of the failure, the error is:

lit.py: /Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg', traceback: Traceback (most recent call last):
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/test/lit.cfg", line 52, in <module>
    configuration.configure()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 129, in configure
    self.configure_triple()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1073, in configure_triple
    self.use_deployment = not self.use_target and self.can_use_deployment()
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/config.py", line 1059, in can_use_deployment
    if not self.target_info.is_host_macosx():
  File "/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/libcxx/utils/libcxx/test/target_info.py", line 76, in is_host_macosx
    name = lit.util.capture(['sw_vers', '-productName']).strip()
AttributeError: 'module' object has no attribute 'capture'

Any ideas on how to fix it?

[1]: http://green.lab.llvm.org/green/job/clang-stage1-configure-RA_check/32959/consoleFull#-42777206a1ca8a51-895e-46c6-af87-ce24fa4cd561

Indeed, that is different... that's a very surprising place for a dependency on lit. I don't think the utility is needed at all here.

This patch should fix it:
https://reviews.llvm.org/D34841

However, I don't believe I have access to a testing environment that can validate the forward fix (or a revert, for that matter). If you want, you can patch it and try it. Otherwise, let me know if I should go ahead and submit with crossed fingers.

mgorny added a subscriber: mgorny.Jul 25 2017, 3:10 PM

This broke two lit tests:

lit :: test-data.py
lit :: unit/TestRunner.py

They both explicitly use FileBasedTest. I'm surprised nobody noticed that because even a most trivial grep would point that out.

I'm sorry, I was wrong. It breaks four tests:

lit :: test-data.py
lit :: test-output.py
lit :: unit/TestRunner.py
lit :: xunit-output.py

I submitted a revert for review: https://reviews.llvm.org/D35877. As described in my comments in https://bugs.llvm.org/show_bug.cgi?id=33704, the diff I submitted is one of three that should get the lit test suite passing again -- reviews greatly appreciated! :)