Enhance Dynamic Include Path Management in xeus-cpp with XEUS_SEARCH_PATH Support #187

AhmedFatthy1040 · 2024-11-28T16:39:17Z

Description

This PR introduces improvements to the xeus-cpp library to handle dynamic include paths more effectively. It includes updates to the interpreter::init_includes function and the CMakeLists file. These changes aim to enhance flexibility and maintainability by leveraging the XEUS_SEARCH_PATH environment variable for managing include directories dynamically.

Key Changes

Updated interpreter::init_includes Function:
- Parses the XEUS_SEARCH_PATH environment variable to include non-standard directories.
- Adds support for multiple include paths, separated by colons (:).
Modified CMakeLists File:
- Dynamically builds the XEUS_SEARCH_PATH variable by joining target include directories.
- Includes the src directory for non-EMSCRIPTEN builds.
- Passes the XEUS_SEARCH_PATH as a preprocessor definition to the compiler.

These updates simplify the configuration and usage of custom include paths.

Fixes #175

Type of change

Please tick all options which are relevant.

Bug fix
New feature
Added/removed dependencies
Required documentation updates

This change: 1. Replaces compile-time XEUS_SEARCH_PATH define with runtime environment variable 2. Removes CMake definition to eliminate redefinition warnings 3. Adds test to verify non-standard include path functionality 4. Makes include path configuration more flexible (can be changed without recompiling) Resolves compiler-research#175

AhmedFatthy1040 · 2024-12-03T21:36:47Z

I have pushed new changes, please approve the awaiting workflows @vgvassilev

AhmedFatthy1040 · 2024-12-04T12:46:34Z

@JohanMabille @anutosh491

anutosh491 · 2024-12-04T12:47:58Z

Hi,

I shall find the time to review this soon !

codecov-commenter · 2024-12-06T21:31:51Z

Codecov Report

Attention: Patch coverage is 87.23404% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.44%. Comparing base (f98cca3) to head (944c962).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/xinterpreter.cpp	87.23%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
- Coverage   80.80%   80.44%   -0.36%     
==========================================
  Files          19       19              
  Lines         974      987      +13     
  Branches       93       97       +4     
==========================================
+ Hits          787      794       +7     
- Misses        187      193       +6

Files with missing lines	Coverage Δ
src/xinterpreter.cpp	`88.94% <87.23%> (-2.46%)`	⬇️

Files with missing lines	Coverage Δ
src/xinterpreter.cpp	`88.94% <87.23%> (-2.46%)`	⬇️

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-06T21:34:33Z

src/xinterpreter.cpp

        Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str());
+
+        // Get include paths from environment variable
+        const char* non_standard_paths = std::getenv("XEUS_SEARCH_PATH");


warning: no header providing "std::getenv" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:19:

- #ifndef EMSCRIPTEN + #include <cstdlib> + #ifndef EMSCRIPTEN

github-actions · 2024-12-06T21:34:34Z

src/xinterpreter.cpp

+            non_standard_paths = "";
+        }
+
+        if (std::strlen(non_standard_paths) > 0)


warning: no header providing "std::strlen" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:19:

- #ifndef EMSCRIPTEN + #include <cstring> + #ifndef EMSCRIPTEN

github-actions · 2024-12-06T21:34:34Z

src/xinterpreter.cpp

+        if (std::strlen(non_standard_paths) > 0)
+        {
+            // Split the paths by colon ':' and add each one
+            std::istringstream stream(non_standard_paths);


warning: no header providing "std::istringstream" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:19:

- #ifndef EMSCRIPTEN + #include <sstream> + #ifndef EMSCRIPTEN

github-actions · 2024-12-06T21:34:34Z

src/xinterpreter.cpp

+            // Split the paths by colon ':' and add each one
+            std::istringstream stream(non_standard_paths);
+            std::string path;
+            while (std::getline(stream, path, ':'))


warning: no header providing "std::getline" is directly included [misc-include-cleaner]

while (std::getline(stream, path, ':')) ^

- Add <cstring> for std::strlen - Add <sstream> for std::istringstream - Add <string> for std::getline Resolves include-cleaner warnings by explicitly including headers for all used standard library symbols. This improves code maintainability and prevents reliance on indirect includes.

AhmedFatthy1040 · 2024-12-09T00:42:38Z

I have fixed the warnings.

github-actions

clang-tidy made some suggestions

src/xinterpreter.cpp

vgvassilev · 2024-12-09T07:07:56Z

CMakeLists.txt

+
+    # Find doctest package
+    find_package(doctest REQUIRED)
+
+    # Test for non-standard include paths
+    add_executable(test_include_paths test/test_include_paths.cpp)
+    target_link_libraries(test_include_paths PRIVATE doctest::doctest)
+    target_include_directories(test_include_paths PRIVATE 
+        ${CMAKE_SOURCE_DIR}/test/custom_includes
+        ${DOCTEST_INCLUDE_DIR}
+    )
+    target_compile_definitions(test_include_paths PRIVATE DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN)
+    add_test(NAME test_include_paths COMMAND test_include_paths)


This block should go in the test folder.

vgvassilev · 2024-12-09T07:16:58Z

src/xinterpreter.cpp

+        // Get include paths from environment variable
+        const char* non_standard_paths = std::getenv("XEUS_SEARCH_PATH");
+        if (!non_standard_paths) {
+            non_standard_paths = "";
+        }
+
+        if (std::strlen(non_standard_paths) > 0)
+        {
+            // Split the paths by colon ':' and add each one
+            std::istringstream stream(non_standard_paths);
+            std::string path;
+            while (std::getline(stream, path, ':'))
+            {
+                if (!path.empty())
+                {
+                    Cpp::AddIncludePath(path.c_str());
+                }
+            }
+        }


We can simplify this by a lot, reduce the mount braces and also consider non-unix path delimiters.

- Relocated the test_include_paths executable setup from the root CMakeLists.txt to the test folder's CMakeLists.txt. - Included the necessary doctest package configuration and non-standard include paths. - Ensured proper target linking and definitions for the test_include_paths target.

github-actions · 2024-12-10T10:39:30Z

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

AhmedFatthy1040 · 2024-12-10T22:18:32Z

I have fixed the clang-format problem.

vgvassilev · 2024-12-12T14:05:35Z

src/xinterpreter.cpp

+        while (std::getline(stream, path, path_separator))
+        {
+            if (!path.empty())
+            {
+                Cpp::AddIncludePath(path.c_str());
+            }
+        }


Suggested change

while (std::getline(stream, path, path_separator))

{

if (!path.empty())

{

Cpp::AddIncludePath(path.c_str());

}

}

while (std::getline(stream, path, path_separator))

if (!path.empty())

Cpp::AddIncludePath(path.c_str());

Apologies, I initially thought the braces were necessary for the clang-tidy format check to pass. I've now edited and correctly formatted the file.

github-actions · 2024-12-12T14:10:01Z

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

AhmedFatthy1040 · 2024-12-16T15:24:03Z

Please approve the awaiting workflows. @anutosh491 @vgvassilev

github-actions · 2024-12-16T15:36:16Z

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

AhmedFatthy1040 · 2024-12-16T15:48:19Z

IDK why it's failing now.

mcbarton · 2024-12-19T09:52:33Z

@AhmedFatthy1040 please squash the commits of the PR and perform git-clang-format on the single commit. Then the ci should pass. Then another review can be made.

AhmedFatthy1040 · 2024-12-19T18:28:47Z

@mcbarton I previously addressed this issue by adding braces here. However, @vgvassilev recommended removing them for code simplification, which I implemented. Unfortunately, this removal triggered a failure in the git clang-format check. I have now corrected the issue by re-adding the braces. Please approve the pending workflows.

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-19T18:39:36Z

src/xinterpreter.cpp

+        std::istringstream stream(non_standard_paths);
+        std::string path;
+        while (std::getline(stream, path, path_separator))
+        {


warning: expected expression [clang-diagnostic-error]

+ { ^

mcbarton · 2024-12-19T18:48:09Z

@mcbarton I previously addressed this issue by adding braces here. However, @vgvassilev recommended removing them for code simplification, which I implemented. Unfortunately, this removal triggered a failure in the git clang-format check. I have now corrected the issue by re-adding the braces. Please approve the pending workflows.

@AhmedFatthy1040 Please revert this change. Squash the commits into 1 commit, then perform git-clang-format on this single commit. Then well see if the workflows pass. We can then review it from that point.

AhmedFatthy1040 · 2024-12-19T18:51:47Z

Sorry for the mistake in the last commit, i am fixing it now.

AhmedFatthy1040 · 2024-12-19T18:52:52Z

@mcbarton I previously addressed this issue by adding braces here. However, @vgvassilev recommended removing them for code simplification, which I implemented. Unfortunately, this removal triggered a failure in the git clang-format check. I have now corrected the issue by re-adding the braces. Please approve the pending workflows.

@AhmedFatthy1040 Please revert this change. Squash the commits into 1 commit, then perform git-clang-format on this single commit. Then well see if the workflows pass. We can then review it from that point.

okay

AhmedFatthy1040 · 2024-12-19T20:13:51Z

I found out that i was using an old version of clang-format, so i updated to version 18 as it is the version used in the CI/CD pipeline, and formatted the code again @mcbarton

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 15. Check the log or trigger a new build to see more.

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

+{
+    Args ClangArgs = {/*"-xc++"*/ "-v"};  // ? {"-Xclang", "-emit-llvm-only", "-Xclang",
+                                          // "-diagnostic-log-file", "-Xclang", "-", "-xc++"};
+    if (std::find_if(


warning: no header providing "std::find_if" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:17:

- #include <cstring> // for std::strlen + #include <algorithm> + #include <cstring> // for std::strlen

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

+        )
+        == ExtraArgs.end())
+    {
+        std::string resource_dir = Cpp::DetectResourceDir();


warning: no header providing "Cpp::DetectResourceDir" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:17:

- #include <cstring> // for std::strlen + #include <clang/Interpreter/CppInterOp.h> + #include <cstring> // for std::strlen

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

+        std::string resource_dir = Cpp::DetectResourceDir();
+        if (resource_dir.empty())
+        {
+            std::cerr << "Failed to detect the resource-dir\n";


warning: no header providing "std::cerr" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:18:

- #include <sstream> // for std::istringstream + #include <iostream> + #include <sstream> // for std::istringstream

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

+        ClangArgs.push_back(resource_dir.c_str());
+    }
+    std::vector<std::string> CxxSystemIncludes;
+    Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes);


warning: no header providing "Cpp::DetectSystemCompilerIncludePaths" is directly included [misc-include-cleaner]

Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes); ^

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

+    ClangArgs.insert(ClangArgs.end(), ExtraArgs.begin(), ExtraArgs.end());
+    // FIXME: We should process the kernel input options and conditionally pass
+    // the gpu args here.
+    return Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/);


warning: no header providing "Cpp::CreateInterpreter" is directly included [misc-include-cleaner]

return Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/); ^

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

-        err = Cpp::EndStdStreamCapture();
-        std::cout << out;
-      }
+    struct StreamRedirectRAII


warning: class 'StreamRedirectRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

struct StreamRedirectRAII ^

github-actions · 2024-12-19T20:35:27Z

src/xinterpreter.cpp

-      }
+    struct StreamRedirectRAII
+    {
+        std::string& err;


warning: member 'err' of type 'std::string &' (aka 'basic_string &') is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

std::string& err; ^

github-actions · 2024-12-19T20:35:28Z

src/xinterpreter.cpp

+        StreamRedirectRAII(std::string& e)
+            : err(e)
+        {
+            Cpp::BeginStdStreamCapture(Cpp::kStdErr);


warning: no header providing "Cpp::BeginStdStreamCapture" is directly included [misc-include-cleaner]

Cpp::BeginStdStreamCapture(Cpp::kStdErr); ^

github-actions · 2024-12-19T20:35:28Z

src/xinterpreter.cpp

+        StreamRedirectRAII(std::string& e)
+            : err(e)
+        {
+            Cpp::BeginStdStreamCapture(Cpp::kStdErr);


warning: no header providing "Cpp::kStdErr" is directly included [misc-include-cleaner]

Cpp::BeginStdStreamCapture(Cpp::kStdErr); ^

github-actions · 2024-12-19T20:35:28Z

src/xinterpreter.cpp

+            : err(e)
+        {
+            Cpp::BeginStdStreamCapture(Cpp::kStdErr);
+            Cpp::BeginStdStreamCapture(Cpp::kStdOut);


warning: no header providing "Cpp::kStdOut" is directly included [misc-include-cleaner]

Cpp::BeginStdStreamCapture(Cpp::kStdOut); ^

AhmedFatthy1040 · 2024-12-19T22:10:08Z

I will try to fix the clang-tidy suggestions tomorrow. Additionally, there was a block of code that was refactored, but it got deleted by mistake when I reset some commits. I will fix that as well.

AhmedFatthy1040 added 2 commits November 28, 2024 18:30

Enhance init_includes to handle dynamic include paths

2a6dacc

Update CMakeLists to support dynamic inclusion of directories

3050471

AhmedFatthy1040 mentioned this pull request Nov 28, 2024

Adjust include path search according to dependencies #175

Open

vgvassilev requested review from JohanMabille and anutosh491 November 28, 2024 18:34

github-actions bot reviewed Dec 6, 2024

View reviewed changes

github-actions bot reviewed Dec 9, 2024

View reviewed changes

src/xinterpreter.cpp Show resolved Hide resolved

vgvassilev reviewed Dec 9, 2024

View reviewed changes

AhmedFatthy1040 requested a review from vgvassilev December 12, 2024 14:01

vgvassilev reviewed Dec 12, 2024

View reviewed changes

AhmedFatthy1040 requested a review from vgvassilev December 12, 2024 15:54

github-actions bot reviewed Dec 19, 2024

View reviewed changes

AhmedFatthy1040 force-pushed the issue-175-dynamic-include-paths branch from c508161 to bdb66eb Compare December 19, 2024 19:42

Formated the code using clang-format-18

944c962

github-actions bot reviewed Dec 19, 2024

View reviewed changes

Enhance Dynamic Include Path Management in xeus-cpp with XEUS_SEARCH_PATH Support #187

Are you sure you want to change the base?

Enhance Dynamic Include Path Management in xeus-cpp with XEUS_SEARCH_PATH Support #187

Conversation

AhmedFatthy1040 commented Nov 28, 2024

Description

Key Changes

Type of change

AhmedFatthy1040 commented Dec 3, 2024

AhmedFatthy1040 commented Dec 4, 2024

anutosh491 commented Dec 4, 2024

codecov-commenter commented Dec 6, 2024 • edited Loading

Codecov Report

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 6, 2024

Choose a reason for hiding this comment

github-actions bot Dec 6, 2024

Choose a reason for hiding this comment

github-actions bot Dec 6, 2024

Choose a reason for hiding this comment

github-actions bot Dec 6, 2024

Choose a reason for hiding this comment

AhmedFatthy1040 commented Dec 9, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

vgvassilev Dec 9, 2024

Choose a reason for hiding this comment

vgvassilev Dec 9, 2024

Choose a reason for hiding this comment

AhmedFatthy1040 Dec 9, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 10, 2024

AhmedFatthy1040 commented Dec 10, 2024

vgvassilev Dec 12, 2024

Choose a reason for hiding this comment

AhmedFatthy1040 Dec 12, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2024

AhmedFatthy1040 commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

AhmedFatthy1040 commented Dec 16, 2024 • edited Loading

mcbarton commented Dec 19, 2024

AhmedFatthy1040 commented Dec 19, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

mcbarton commented Dec 19, 2024

AhmedFatthy1040 commented Dec 19, 2024

AhmedFatthy1040 commented Dec 19, 2024

AhmedFatthy1040 commented Dec 19, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

github-actions bot Dec 19, 2024

Choose a reason for hiding this comment

AhmedFatthy1040 commented Dec 19, 2024

codecov-commenter commented Dec 6, 2024 •

edited

Loading

AhmedFatthy1040 commented Dec 16, 2024 •

edited

Loading