Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ message(STATUS "Building xeus-cpp v${${PROJECT_NAME}_VERSION}")
# Build options
# =============

# Note: XEUS_SEARCH_PATH is now handled at runtime through environment variables

option(XEUS_CPP_BUILD_STATIC "Build xeus-cpp static library" ON)
option(XEUS_CPP_BUILD_SHARED "Split xcpp build into executable and library" ON)
option(XEUS_CPP_BUILD_EXECUTABLE "Build the xcpp executable" ON)
Expand Down Expand Up @@ -404,6 +406,7 @@ endif()
# =====

if(XEUS_CPP_BUILD_TESTS)
enable_testing()
add_subdirectory(test)
endif()

Expand Down
136 changes: 93 additions & 43 deletions src/xinterpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
#include "xeus-cpp/xinterpreter.hpp"
#include "xeus-cpp/xmagics.hpp"

#include <cstring> // for std::strlen
#include <sstream> // for std::istringstream
#include <string> // for std::getline

#include "xinput.hpp"
#include "xinspect.hpp"
#ifndef EMSCRIPTEN
Expand All @@ -26,43 +30,62 @@

using Args = std::vector<const char*>;

void* createInterpreter(const Args &ExtraArgs = {}) {
Args ClangArgs = {/*"-xc++"*/"-v"}; // ? {"-Xclang", "-emit-llvm-only", "-Xclang", "-diagnostic-log-file", "-Xclang", "-", "-xc++"};
if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) {
return s == "-resource-dir";}) == ExtraArgs.end()) {
std::string resource_dir = Cpp::DetectResourceDir();
if (resource_dir.empty())
std::cerr << "Failed to detect the resource-dir\n";
ClangArgs.push_back("-resource-dir");
ClangArgs.push_back(resource_dir.c_str());
}
std::vector<std::string> CxxSystemIncludes;
Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes);
for (const std::string& CxxInclude : CxxSystemIncludes) {
ClangArgs.push_back("-isystem");
ClangArgs.push_back(CxxInclude.c_str());
}
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"}*/);
void* createInterpreter(const Args& ExtraArgs = {})
{
Args ClangArgs = {/*"-xc++"*/ "-v"}; // ? {"-Xclang", "-emit-llvm-only", "-Xclang",
// "-diagnostic-log-file", "-Xclang", "-", "-xc++"};
if (std::find_if(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ExtraArgs.begin(),
ExtraArgs.end(),
[](const std::string& s)
{
return s == "-resource-dir";
}
)
== ExtraArgs.end())
{
std::string resource_dir = Cpp::DetectResourceDir();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (resource_dir.empty())
{
std::cerr << "Failed to detect the resource-dir\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
ClangArgs.push_back("-resource-dir");
ClangArgs.push_back(resource_dir.c_str());
}
std::vector<std::string> CxxSystemIncludes;
Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

    Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes);
         ^

for (const std::string& CxxInclude : CxxSystemIncludes)
{
ClangArgs.push_back("-isystem");
ClangArgs.push_back(CxxInclude.c_str());
}
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"}*/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

using namespace std::placeholders;

namespace xcpp
{
struct StreamRedirectRAII {
std::string &err;
StreamRedirectRAII(std::string &e) : err(e) {
Cpp::BeginStdStreamCapture(Cpp::kStdErr);
Cpp::BeginStdStreamCapture(Cpp::kStdOut);
}
~StreamRedirectRAII() {
std::string out = Cpp::EndStdStreamCapture();
err = Cpp::EndStdStreamCapture();
std::cout << out;
}
struct StreamRedirectRAII
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
           ^

{
std::string& err;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

        std::string& err;
                     ^


StreamRedirectRAII(std::string& e)
: err(e)
{
Cpp::BeginStdStreamCapture(Cpp::kStdErr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Cpp::BeginStdStreamCapture(Cpp::kStdOut);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

~StreamRedirectRAII()
{
std::string out = Cpp::EndStdStreamCapture();
err = Cpp::EndStdStreamCapture();
std::cout << out;
}
};

void interpreter::configure_impl()
Expand Down Expand Up @@ -98,15 +121,14 @@ __get_cxx_version ()
return std::to_string(cxx_version);
}


interpreter::interpreter(int argc, const char* const* argv) :
xmagics()
interpreter::interpreter(int argc, const char* const* argv)
: xmagics()
, p_cout_strbuf(nullptr)
, p_cerr_strbuf(nullptr)
, m_cout_buffer(std::bind(&interpreter::publish_stdout, this, _1))
, m_cerr_buffer(std::bind(&interpreter::publish_stderr, this, _1))
{
//NOLINTNEXTLINE (cppcoreguidelines-pro-bounds-pointer-arithmetic)
// NOLINTNEXTLINE (cppcoreguidelines-pro-bounds-pointer-arithmetic)
createInterpreter(Args(argv ? argv + 1 : argv, argv + argc));
m_version = get_stdopt();
redirect_output();
Expand Down Expand Up @@ -210,10 +232,11 @@ __get_cxx_version ()
//
// JupyterLab displays the "{ename}: {evalue}" if the traceback is
// empty.
if (evalue.size() < 4) {
if (evalue.size() < 4)
{
ename = " ";
}
std::vector<std::string> traceback({ename + evalue});
std::vector<std::string> traceback({ename + evalue});
if (!config.silent)
{
publish_execution_error(ename, evalue, traceback);
Expand Down Expand Up @@ -256,7 +279,8 @@ __get_cxx_version ()

Cpp::CodeComplete(results, code.c_str(), 1, _cursor_pos + 1);

return xeus::create_complete_reply(results /*matches*/,
return xeus::create_complete_reply(
results /*matches*/,
cursor_pos - to_complete.length() /*cursor_start*/,
cursor_pos /*cursor_end*/
);
Expand All @@ -277,13 +301,17 @@ __get_cxx_version ()

nl::json interpreter::is_complete_request_impl(const std::string& code)
{
if (!code.empty() && code[code.size() - 1] == '\\') {
if (!code.empty() && code[code.size() - 1] == '\\')
{
auto found = code.rfind('\n');
if (found == std::string::npos)
{
found = -1;
}
auto found1 = found++;
while (isspace(code[++found1])) ;
return xeus::create_is_complete_reply("incomplete", code.substr(found, found1-found));
while (isspace(code[++found1]))
;
return xeus::create_is_complete_reply("incomplete", code.substr(found, found1 - found));
}

return xeus::create_is_complete_reply("complete");
Expand Down Expand Up @@ -357,16 +385,38 @@ __get_cxx_version ()

void interpreter::init_includes()
{
// Add the standard include path
Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str());

// Get include paths from environment variable
const char* non_standard_paths = std::getenv("XEUS_SEARCH_PATH");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/xinterpreter.cpp:19:

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

AhmedFatthy1040 marked this conversation as resolved.
Show resolved Hide resolved
if (!non_standard_paths)
{
non_standard_paths = "";
}

if (std::strlen(non_standard_paths) > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/xinterpreter.cpp:19:

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

{
// Split the paths by colon ':' and add each one
std::istringstream stream(non_standard_paths);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/xinterpreter.cpp:19:

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

std::string path;
while (std::getline(stream, path, ':'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

{
if (!path.empty())
{
Cpp::AddIncludePath(path.c_str());
}
}
}
}

void interpreter::init_preamble()
{
//NOLINTBEGIN(cppcoreguidelines-owning-memory)
// NOLINTBEGIN(cppcoreguidelines-owning-memory)
preamble_manager.register_preamble("introspection", std::make_unique<xintrospection>());
preamble_manager.register_preamble("magics", std::make_unique<xmagics_manager>());
preamble_manager.register_preamble("shell", std::make_unique<xsystem>());
//NOLINTEND(cppcoreguidelines-owning-memory)
// NOLINTEND(cppcoreguidelines-owning-memory)
}

void interpreter::init_magic()
Expand Down
18 changes: 18 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,21 @@ target_link_libraries(test_xeus_cpp xeus-cpp doctest::doctest ${CMAKE_THREAD_LIB
target_include_directories(test_xeus_cpp PRIVATE ${XEUS_CPP_INCLUDE_DIR})

add_custom_target(xtest COMMAND test_xeus_cpp DEPENDS test_xeus_cpp)

# Test for non-standard include paths
add_executable(test_include_paths 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)

set(XEUS_SEARCH_PATH $<JOIN:$<TARGET_PROPERTY:xeus-cpp,INCLUDE_DIRECTORIES>,:>)

if (NOT EMSCRIPTEN)
set(XEUS_SEARCH_PATH "${XEUS_SEARCH_PATH}:${CMAKE_CURRENT_SOURCE_DIR}/src")
endif()

target_compile_definitions(xeus-cpp PRIVATE "XEUS_SEARCH_PATH=\"${XEUS_SEARCH_PATH}\"")
8 changes: 8 additions & 0 deletions test/custom_includes/test_header.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#ifndef TEST_HEADER_HPP
#define TEST_HEADER_HPP

namespace test_ns {
constexpr int test_value = 42;
}

#endif
8 changes: 8 additions & 0 deletions test/test_include_paths.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include <doctest/doctest.h>
#include <string>
#include "test_header.hpp"

TEST_CASE("Test non-standard include paths")
{
CHECK(test_ns::test_value == 42);
}