Page MenuHomePhabricator

[LIT] Add new LLDB test format
AbandonedPublic

Authored by JDevlieghere on Apr 5 2018, 1:37 PM.

Details

Summary

This adds a new test format to lit which will be used for LLDB.

(If it's possible to define a new format outside the LLVM repo, I'm more than happy to move it into the LLDB repo)

Diff Detail

Event Timeline

JDevlieghere created this revision.Apr 5 2018, 1:37 PM

It should be possible to define it outside the LLVM repo. Just in llvm/lldb/lit/lit.cfg replace this line:

config.test_format = lit.formats.ShTest(execute_external)

with something like this:

import lldb_format
config.test_format = lldb_format.LLDBTestFormat()

and that file can live in the lldb repo.

It should be possible to define it outside the LLVM repo. Just in llvm/lldb/lit/lit.cfg replace this line:

config.test_format = lit.formats.ShTest(execute_external)

with something like this:

import lldb_format
config.test_format = lldb_format.LLDBTestFormat()

and that file can live in the lldb repo.

Probably I'm doing something wrong then because I couldn't get that to work. I moved the lldbtest.py in the Suite dir and then I got an import error.

ImportError: No module named lldbtest

I don't think sys.path is set up correctly to be able to find the lldbtest package from the lldb/lit folder.

These things kind of evolved separately, and the lldb/lit folder was created as a place to start iterating on LLVM-style lit / FileCheck tests. These kind of tests -- by definition -- don't really use the SB API, so no work was ever done to set up paths correctly so that it could write import lldb or to re-use any of the other stuff from packages/Python.

I'm not sure what the best thing to do is, but usually the canonical structuring is to have the test files in the same tree as the lit configuration. So perhaps you could put a lit configuration file in lldb/packages/Python/lldbsuite and have that be separate from lldb/lit, with the goal of eventually (possibly) merging them. Then have a separate CMake target so you'd still have check-lldb-lit which goes into the lldb/lit directory, and another one like check-lldb-lit-dotest which starts from the lldb/packages/Python/lldbsuite directory.

On the other hand, if you want to see how dotest.py sets up its sys.path, have a look at lldb/test/dotest.py The magic is in this use_lldb_suite function, which walks backwards through the tree until it finds the root, then dives into the lldbsuite folder to manually add it to sys.path.

I don't think sys.path is set up correctly to be able to find the lldbtest package from the lldb/lit folder.

These things kind of evolved separately, and the lldb/lit folder was created as a place to start iterating on LLVM-style lit / FileCheck tests. These kind of tests -- by definition -- don't really use the SB API, so no work was ever done to set up paths correctly so that it could write import lldb or to re-use any of the other stuff from packages/Python.

I'm not sure what the best thing to do is, but usually the canonical structuring is to have the test files in the same tree as the lit configuration. So perhaps you could put a lit configuration file in lldb/packages/Python/lldbsuite and have that be separate from lldb/lit, with the goal of eventually (possibly) merging them. Then have a separate CMake target so you'd still have check-lldb-lit which goes into the lldb/lit directory, and another one like check-lldb-lit-dotest which starts from the lldb/packages/Python/lldbsuite directory.

On the other hand, if you want to see how dotest.py sets up its sys.path, have a look at lldb/test/dotest.py The magic is in this use_lldb_suite function, which walks backwards through the tree until it finds the root, then dives into the lldbsuite folder to manually add it to sys.path.

Do you feel all that outweighs the alternative of just having the format in llvm/Utils as is the case in this diff? We already have some LLDB specific stuff there and I would argue that conceptually it makes (at least a little) sense to have all the format living together.

labath added a comment.Apr 6 2018, 1:51 AM

I don't think sys.path is set up correctly to be able to find the lldbtest package from the lldb/lit folder.

These things kind of evolved separately, and the lldb/lit folder was created as a place to start iterating on LLVM-style lit / FileCheck tests. These kind of tests -- by definition -- don't really use the SB API, so no work was ever done to set up paths correctly so that it could write import lldb or to re-use any of the other stuff from packages/Python.

I'm not sure what the best thing to do is, but usually the canonical structuring is to have the test files in the same tree as the lit configuration. So perhaps you could put a lit configuration file in lldb/packages/Python/lldbsuite and have that be separate from lldb/lit, with the goal of eventually (possibly) merging them. Then have a separate CMake target so you'd still have check-lldb-lit which goes into the lldb/lit directory, and another one like check-lldb-lit-dotest which starts from the lldb/packages/Python/lldbsuite directory.

On the other hand, if you want to see how dotest.py sets up its sys.path, have a look at lldb/test/dotest.py The magic is in this use_lldb_suite function, which walks backwards through the tree until it finds the root, then dives into the lldbsuite folder to manually add it to sys.path.

Do you feel all that outweighs the alternative of just having the format in llvm/Utils as is the case in this diff? We already have some LLDB specific stuff there and I would argue that conceptually it makes (at least a little) sense to have all the format living together.

I don't think it needs to be that complex.

You already have LLDB_SOURCE_DIR from lit.site.cfg.in, so it should only be a matter of doing something like this in lit.cfg:

sys.path.append(os.path.join(config.lldb_src_root, "whereever_is_the_format_file")
from something import LLDBTest
config.test_format = LLDBTest(...)

If we can make things as simple as this, then I think we should move this to the lldb repo.

utils/lit/lit/formats/lldbtest.py
32

Do we need the suffixes to be configurable? The whole testsuite is very specific to python and I don't see us running non-python files through this method any time soon.

60

This line only appears if you run dotest in --no-multiprocess mode, but I don't see where you are adding that. Am I missing something?

JDevlieghere abandoned this revision.Apr 9 2018, 3:28 AM

I don't think sys.path is set up correctly to be able to find the lldbtest package from the lldb/lit folder.

These things kind of evolved separately, and the lldb/lit folder was created as a place to start iterating on LLVM-style lit / FileCheck tests. These kind of tests -- by definition -- don't really use the SB API, so no work was ever done to set up paths correctly so that it could write import lldb or to re-use any of the other stuff from packages/Python.

I'm not sure what the best thing to do is, but usually the canonical structuring is to have the test files in the same tree as the lit configuration. So perhaps you could put a lit configuration file in lldb/packages/Python/lldbsuite and have that be separate from lldb/lit, with the goal of eventually (possibly) merging them. Then have a separate CMake target so you'd still have check-lldb-lit which goes into the lldb/lit directory, and another one like check-lldb-lit-dotest which starts from the lldb/packages/Python/lldbsuite directory.

On the other hand, if you want to see how dotest.py sets up its sys.path, have a look at lldb/test/dotest.py The magic is in this use_lldb_suite function, which walks backwards through the tree until it finds the root, then dives into the lldbsuite folder to manually add it to sys.path.

Do you feel all that outweighs the alternative of just having the format in llvm/Utils as is the case in this diff? We already have some LLDB specific stuff there and I would argue that conceptually it makes (at least a little) sense to have all the format living together.

I don't think it needs to be that complex.

You already have LLDB_SOURCE_DIR from lit.site.cfg.in, so it should only be a matter of doing something like this in lit.cfg:

sys.path.append(os.path.join(config.lldb_src_root, "whereever_is_the_format_file")
from something import LLDBTest
config.test_format = LLDBTest(...)

If we can make things as simple as this, then I think we should move this to the lldb repo.

Thanks, that did the trick :-)