Implement `GetIncludePaths` #311

Vipul-Cariappa · 2024-07-27T10:42:12Z

Fixes #69

I was unsure if to return std::vector<std::string> or a single std::string with delimiter. I have gone with a single std::string with : as the delimiter, but it can be changed.

With the following changes in the cppyy library.

diff --git a/python/cppyy/__init__.py b/python/cppyy/__init__.py
index 53b7e92..441c34c 100644
--- a/python/cppyy/__init__.py
+++ b/python/cppyy/__init__.py
@@ -260,6 +260,11 @@ def add_include_path(path):
         raise OSError('No such directory: %s' % path)
     gbl.Cpp.AddIncludePath(path)
 
+def get_include_path():
+    """Get include paths available to Cling."""
+    path = gbl.Cpp.GetIncludePaths()
+    return path.split(":")
+
 def add_library_path(path):
     """Add a path to the library search paths available to Cling."""
     if not os.path.isdir(path):

We can get the include paths in Python

>>> import cppyy
>>> print(*cppyy.get_include_path(), sep="\n")
.
/home/vipul/Workspace/c/llvm-project/llvm/include
/home/vipul/Workspace/c/llvm-project/clang/include
/home/vipul/Workspace/c/llvm-project/build/include
/home/vipul/Workspace/c/llvm-project/build/tools/clang/include
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/x86_64-redhat-linux
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/backward
/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/include
/usr/local/include
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../x86_64-redhat-linux/include
/include
/usr/include
/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/../../../../cling-src/tools/cling/include
/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/../../../../cling-src/include
/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/../../..//include
/home/vipul/Workspace/c/cppyy-backend-compiler-research/python/cppyy_backend/include
/home/vipul/micromamba/envs/compiler-research/include/python3.12
/home/vipul/micromamba/envs/compiler-research/lib/python3.12/site-packages/../../../include/python3.12
/home/vipul/micromamba/envs/compiler-research/include

vgvassilev · 2024-07-27T16:15:38Z

Fixes #69

I was unsure if to return std::vector<std::string> or a single std::string with delimiter. I have gone with a single std::string with : as the delimiter, but it can be changed.

Probably we should go for a std::vector<std::string>& as a output parameter.

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOpInterpreter.h

codecov · 2024-07-27T22:27:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.38%. Comparing base (34186d6) to head (5eb82e1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
+ Coverage   72.54%   73.38%   +0.83%     
==========================================
  Files           8        8              
  Lines        2972     2979       +7     
==========================================
+ Hits         2156     2186      +30     
+ Misses        816      793      -23

Files	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.00% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`79.12% <100.00%> (+0.06%)`	⬆️
lib/Interpreter/CppInterOpInterpreter.h	`76.25% <100.00%> (+0.30%)`	⬆️

... and 1 file with indirect coverage changes

Files	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.00% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`79.12% <100.00%> (+0.06%)`	⬆️
lib/Interpreter/CppInterOpInterpreter.h	`76.25% <100.00%> (+0.30%)`	⬆️

... and 1 file with indirect coverage changes

Vipul-Cariappa · 2024-07-28T05:00:24Z

Looking at the failing test, cling has Interpreter::GetIncludePaths function defined as void Interpreter::GetIncludePaths(llvm::SmallVectorImpl<std::string>& incpaths, bool withSystem, bool withFlags) (reference: definition of Interpreter::GetIncludePaths in cling). I guess we need to define Interpreter::GetIncludePaths using the same function signature in CppInterOp too.

That raises a question; How should I handle the withSystem and withFlags arguments? This is what cling does. Should I just copy that code?

I don't completely know/understand some of the options there.

Probably we should go for a std::vector<std::string>& as a output parameter.

Not sure if we can return a reference, because we are not storing it as a vector in the class. We could return std::vector<std::string&>.
We are getting the include paths directly from LLVM using HeaderSearchOptions class.

vgvassilev · 2024-07-28T06:12:46Z

Looking at the failing test, cling has Interpreter::GetIncludePaths function defined as void Interpreter::GetIncludePaths(llvm::SmallVectorImpl<std::string>& incpaths, bool withSystem, bool withFlags) (reference: definition of Interpreter::GetIncludePaths in cling). I guess we need to define Interpreter::GetIncludePaths using the same function signature in CppInterOp too.

That raises a question; How should I handle the withSystem and withFlags arguments? This is what cling does. Should I just copy that code?

That's probably the easiest.

I don't completely know/understand some of the options there.

Probably we should go for a std::vector<std::string>& as a output parameter.

Not sure if we can return a reference, because we are not storing it as a vector in the class. We could return std::vector<std::string&>. We are getting the include paths directly from LLVM using HeaderSearchOptions class.

I meant having a signature like: void GetIncludePaths(std::vector<std::string>& IncludePaths) where we will store the result.

Vipul-Cariappa · 2024-07-28T10:39:05Z

@vgvassilev, please have a look at the recent changes. I have adapted the code directly from cling. The behavior matches cling's behavior.

With the following changes in cppyy

diff --git a/python/cppyy/__init__.py b/python/cppyy/__init__.py
index 53b7e92..d75f048 100644
--- a/python/cppyy/__init__.py
+++ b/python/cppyy/__init__.py
@@ -260,6 +260,12 @@ def add_include_path(path):
         raise OSError('No such directory: %s' % path)
     gbl.Cpp.AddIncludePath(path)
 
+def get_include_path():
+    """Get include paths available to Cling."""
+    paths = gbl.std.vector[gbl.std.string]()
+    gbl.Cpp.GetIncludePaths(paths)
+    return list(map(str, paths))
+
 def add_library_path(path):
     """Add a path to the library search paths available to Cling."""
     if not os.path.isdir(path):

You can use it in Python

>>> import cppyy
>>> cppyy.get_include_path()
['',
 '/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/../../../../cling-src/tools/cling/include',
 '/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/../../../../cling-src/include',
 '/home/vipul/Workspace/c/llvm-project/build/lib/clang/17/../../..//include',
 '/home/vipul/Workspace/c/cppyy-backend-compiler-research/python/cppyy_backend/include',
 '/home/vipul/micromamba/envs/compiler-research/include/python3.12',
 '/home/vipul/micromamba/envs/compiler-research/lib/python3.12/site-packages/../../../include/python3.12',
 '/home/vipul/micromamba/envs/compiler-research/include']

lib/Interpreter/CppInterOp.cpp

vgvassilev · 2024-07-28T12:57:17Z

This looks reasonable. We should add a unit test.

github-actions · 2024-07-28T13:06:38Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-07-29T05:21:17Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-07-29T05:46:49Z

unittests/CppInterOp/InterpreterTest.cpp

@@ -113,6 +113,12 @@ TEST(InterpreterTest, DetectSystemCompilerIncludePaths) {
  EXPECT_FALSE(includes.empty());
 }

+TEST(InterpreterTest, GetIncludePaths) {
+  std::vector<std::string> includes;


We should also test the separate flags and make sure the output is expected.

Vipul-Cariappa · 2024-07-29T06:41:43Z

unittests/CppInterOp/InterpreterTest.cpp

+
+  llvm::SmallVector<std::string> includes(1);
+
+  I->GetIncludePaths(includes, /*withSystem=*/false, /*withFlags=*/false);


Please note the difference in the signature of GetIncludePaths in include/clang/Interpreter/CppInterOp.h: void GetIncludePaths(std::vector<std::string>& IncludePaths) and void Interpreter::GetIncludePaths(llvm::SmallVectorImpl<std::string>& incpaths, bool withSystem, bool withFlags) in lib/Interpreter/CppInterOpInterpreter.h.

Should both of these functions have the same signature?
While retrieving include path from Python using cppyy. The user may also want to get the system path and flags, but cannot with the current implementation.

The Interpreter::GetIncludePaths should be hidden for the user. So I'd not worry about that.

vgvassilev

LGTM!

vgvassilev · 2024-08-05T15:18:25Z

unittests/CppInterOp/InterpreterTest.cpp

+
+  llvm::SmallVector<std::string> includes(1);
+
+  I->GetIncludePaths(includes, /*withSystem=*/false, /*withFlags=*/false);


The Interpreter::GetIncludePaths should be hidden for the user. So I'd not worry about that.

github-actions · 2024-08-05T15:34:35Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-08-05T16:36:57Z

@Vipul-Cariappa, looks like the failures are due to some mismatch between the cling and clang-repl implementation. Maybe that's what you were trying to say in your previous comments. Can you look at failures?

mcbarton · 2024-08-05T18:47:04Z

@Vipul-Cariappa can you rebase this PR, so you can see if any changes are needed to be compatible with llvm 19?

Co-authored-by: Vassil Vassilev <[email protected]>

github-actions · 2024-08-05T19:41:04Z

clang-tidy review says "All clean, LGTM! 👍"

Vipul-Cariappa · 2024-08-11T14:49:15Z

@vgvassilev, I believe the latest commit should fix the failing CI related to cling.
But clang-REPL LLVM 19 test also failed. Not sure if it's due to my changes.
Maybe it won't fail in the next run.

aaronj0 · 2024-08-11T16:54:41Z

@vgvassilev, I believe the latest commit should fix the failing CI related to cling. But clang-REPL LLVM 19 test also failed. Not sure if it's due to my changes. Maybe it won't fail in the next run.

I wouldn't worry about that, It's probably valgrind complaining about clang19 on the cppyy tests(failing on master too) Has been an issue for a while now that I will update with a fix soon

github-actions · 2024-08-11T20:42:17Z

clang-tidy review says "All clean, LGTM! 👍"

aaronj0 · 2024-08-11T21:53:29Z

@Vipul-Cariappa A fix has been patched. I have rerun the failing job, can you rebase into a single commit once they are green

github-actions bot reviewed Jul 27, 2024

View reviewed changes

lib/Interpreter/CppInterOpInterpreter.h Outdated Show resolved Hide resolved

lib/Interpreter/CppInterOpInterpreter.h Outdated Show resolved Hide resolved

vgvassilev reviewed Jul 28, 2024

View reviewed changes

lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved

Vipul-Cariappa requested a review from vgvassilev July 28, 2024 12:56

vgvassilev reviewed Jul 29, 2024

View reviewed changes

Vipul-Cariappa commented Jul 29, 2024

View reviewed changes

Vipul-Cariappa requested a review from vgvassilev July 29, 2024 06:42

vgvassilev approved these changes Aug 5, 2024

View reviewed changes

Vipul-Cariappa and others added 7 commits August 5, 2024 22:23

Implement GetIncludePaths (compiler-research#69)

8a3d2aa

Adapted GetIncludePaths from cling

2e63a20

improved docs

1a423d0

Update CppInterOp.cpp according to code review

b3d9203

Co-authored-by: Vassil Vassilev <[email protected]>

clang format fix

c1f32d6

add unittest

12327ce

add unittest to test with different flags

5062b94

vgvassilev force-pushed the GetIncludePaths branch from 584b80f to 5062b94 Compare August 5, 2024 19:23

fix for cling

5eb82e1

aaronj0 approved these changes Aug 11, 2024

View reviewed changes

vgvassilev merged commit bebebcd into compiler-research:main Aug 12, 2024
59 checks passed

Vipul-Cariappa deleted the GetIncludePaths branch September 28, 2024 04:16

Vipul-Cariappa mentioned this pull request Dec 18, 2024

Add support for GetIncludePaths #178

Closed


		llvm::SmallVector<std::string> includes(1);

		I->GetIncludePaths(includes, /withSystem=/false, /withFlags=/false);

Implement GetIncludePaths #311

Implement GetIncludePaths #311

Uh oh!

Conversation

Vipul-Cariappa commented Jul 27, 2024

Uh oh!

vgvassilev commented Jul 27, 2024

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Vipul-Cariappa commented Jul 28, 2024

Uh oh!

vgvassilev commented Jul 28, 2024

Uh oh!

Vipul-Cariappa commented Jul 28, 2024

Uh oh!

Uh oh!

vgvassilev commented Jul 28, 2024

Uh oh!

github-actions bot commented Jul 28, 2024

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

vgvassilev Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

vgvassilev Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 5, 2024

Uh oh!

vgvassilev commented Aug 5, 2024

Uh oh!

mcbarton commented Aug 5, 2024

Uh oh!

github-actions bot commented Aug 5, 2024

Uh oh!

Vipul-Cariappa commented Aug 11, 2024

Uh oh!

aaronj0 commented Aug 11, 2024

Uh oh!

github-actions bot commented Aug 11, 2024

Uh oh!

aaronj0 commented Aug 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Implement `GetIncludePaths` #311

Implement `GetIncludePaths` #311

codecov bot commented Jul 27, 2024 •

edited

Loading

aaronj0 commented Aug 11, 2024 •

edited

Loading