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 4 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
16 changes: 16 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,7 +406,21 @@ endif()
# =====

if(XEUS_CPP_BUILD_TESTS)
enable_testing()
add_subdirectory(test)

# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This block should go in the test folder.

endif()

# Installation
Expand Down
25 changes: 25 additions & 0 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 Down Expand Up @@ -357,7 +361,28 @@ __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());
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

void interpreter::init_preamble()
Expand Down
8 changes: 8 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,11 @@ 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)

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);
}
Loading