[ExpressionParser] Test GetClangResourceDir

[xiaobai]

Summary

I'm doing this because I plan on implementing ComputeClangDirectory on windows
so that GetClangResourceDir will work. Additionally, I made
test_paths make sure that the directory member of the returned FileSpec is not
none. This will fail on windows since ComputeClangDirectory isn't implemented
yet.

[teemperor]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

Doesn't that mean that we no longer test any of the paths on windows? E.g. if someone breaks lldb.ePathTypeLLDBShlibDir we will not see this test failing on Windows. I would prefer of extracting the ClangDir test into it's own method that we mark as failing.

[xiaobai]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

Technically it is being tested but it is expected to fail. That being said, I don't mind extracting the ClangDir test into its own method while I work on implementing it, but I don't think it needs its own test. Maybe after I implement GetClangResourceDir on windows I can merge the tests? :P

[labath]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

That sounds fine, though I have to say that these tests don't really test much. There is a test for the MacOS functionality here in unittests/Expression/ClangParserTest.cpp, which is much more detailed. It would be great if you could add a windows version there too.

[xiaobai]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

I originally added this test there, but testing things there as-is is presents a disadvantage: The unittests don't link against liblldb but the individual lldb libraries, so the methods that figure out where the Clang Resource Dir (and pretty much any other directory for that matter) will report something like this:
/path/to/llvm/build/tools/lldb/unittests/lib/clang/9.0.0. So, the best this test could do is set that the filespec had a directory and not a filename set, which is what this test already does.

That being said, the way MacOS is being tested is much nicer. I could do a small refactor so the non-MacOS posix implementation is closer to the MacOS implementation and add a Linux test (and eventually a windows test) to unittests/Expression/ClangParserTest.cpp as well. What do you think?

[labath]

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

20

Sounds like a plan.

[xiaobai]

Address suggestions and add unit test

[xiaobai]

Reword comment to be clearer

[xiaobai]

Looks like the test I wrote doesn't pass on MacOS, I need to investigate.

[xiaobai]

Minor changes to make MacOS work

[labath]

Looks fine to me, but maybe @aprantl could give this a quick look, as he's the one (IIRC) who introduced this function.

[xiaobai]

@aprantl: Do you mind taking a look at this when you get a moment?

[aprantl]

Looks mostly good!

source/Plugins/ExpressionParser/Clang/ClangHost.cpp

45 ↗

(On Diff #188952)

While you are at it, could you rename this DefaultComputeClangResourceDirectory?
And add a doxygen comment that says it will compute the calng resource directory assuming that clang was installed to the same prefix as lldb?

[xiaobai]

{Default,}ComputeClangDirectory -> {Default,}ComputeClangResourceDir
Added a comment explaining exactly how DefaultComputeClangResourceDir works

[stella.stamenova]

This introduced a build break on Windows (which happened to coincide with another build break).

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/2317

packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py

 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class TestPaths(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
+    # GetClangResourceDir doesn't work on windows yet
+    @expectedFailureAll(oslist=["windows"])
     @no_debug_info_test
     def test_paths(self):
         '''Test to make sure no file names are set in the lldb.SBFileSpec objects returned by lldb.SBHostOS.GetLLDBPath() for paths that are directories'''
         dir_path_types = [lldb.ePathTypeLLDBShlibDir,
                           lldb.ePathTypeSupportExecutableDir,
                           lldb.ePathTypeHeaderDir,
                           lldb.ePathTypePythonDir,
                           lldb.ePathTypeLLDBSystemPlugins,
                           lldb.ePathTypeLLDBUserPlugins,
-                          lldb.ePathTypeLLDBTempSystemDir]
+                          lldb.ePathTypeLLDBTempSystemDir,
+                          lldb.ePathTypeClangDir]
 
         for path_type in dir_path_types:
             f = lldb.SBHostOS.GetLLDBPath(path_type)
             # No directory path types should have the filename set
             self.assertTrue(f.GetFilename() is None)
+            # All directory path types should have the directory set
+            self.assertFalse(f.GetDirectory() is None)
 
     @no_debug_info_test
     def test_directory_doesnt_end_with_slash(self):
         current_directory_spec = lldb.SBFileSpec(os.path.curdir)
         current_directory_string = current_directory_spec.GetDirectory()
         self.assertNotEqual(current_directory_string[-1:], '/')
         pass