Update tags definition, add tests for xinspect #128
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ Coverage 79.41% 82.05% +2.64%
==========================================
Files 17 17
Lines 612 613 +1
Branches 59 59
==========================================
+ Hits 486 503 +17
+ Misses 126 110 -16
... and 1 file with indirect coverage changes
|
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
tharun571
changed the title
|
clang-tidy review says "All clean, LGTM! 👍" |
tharun571
force-pushed
the
test-coverage
branch
2 times, most recently
from
b8bbb18 to
4535a36
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
CMakeLists.txt
Outdated
| endif () | ||
| endforeach () | ||
| # Set the version | ||
| set(XEUS_CPP_VERSION_MAJOR 0) |
There was a problem hiding this comment.
@tharun571 You probably want to set the version number the way we do in CppInterOp with a version file (see https://github.com/compiler-research/CppInterOp/pull/296/files for more details). It will allow us to label something as a version in development.
There was a problem hiding this comment.
The version file for xeus-cpp would currently have 0.5.0;dev as we are currently developing version 0.5.0 and no longer on version 0.4.0
There was a problem hiding this comment.
You won't need to add a XeusCppConfigVersion.cmake.in to be clear
There was a problem hiding this comment.
We can add a "XEUS_CPP_LABEL" or anything else to handle the "dev" part of the version (when there is one). But as said previously, I am strongly opposed to having cmake injecting the version in the cpp code.
There was a problem hiding this comment.
Looks good to me but I cannot take that decision without @JohanMabille.
@vgvassilev There is a bug at the moment. When the config header file is configured it still has ;dev in the patch variable . Me and @tharun571 have been discussing. I can't see why. Do you see why? |
Do we parse the list properly the way we do in CppInterOp? |
In an identical way as I linked the PR where we parsed it in CppInterOp as a guide. For some reason that isn't obvious to me the patch number still has ;dev . |
|
This ;dev doesn't happen in CppInterOp, as it was the whole reason we parsed it in this way to avoid the ;dev in patch version number, and allowing finding a version number correctly. |
|
Well, something must be different. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
@vgvassilev A solution to the patch version issue has been found and the PR is ready for review by @JohanMabille |
| #define XEUS_CPP_VERSION_PATCH 0 | ||
| #define XEUS_CPP_VERSION_MAJOR @XEUS_CPP_VERSION_MAJOR@ | ||
| #define XEUS_CPP_VERSION_MINOR @XEUS_CPP_VERSION_MINOR@ | ||
| #define XEUS_CPP_VERSION_PATCH @XEUS_CPP_VERSION_PATCH@ |
There was a problem hiding this comment.
This should not be configured by an external tool. This makes the cpp code depend on the building tools. This should be reverted to the previous behavior which was the correct one.
CMakeLists.txt
Outdated
| string(REPLACE "\\" "" VERSION_LIST "${VERSION_LIST}") | ||
| list(GET VERSION_LIST 0 XEUS_CPP_VERSION_MAJOR) | ||
| list(GET VERSION_LIST 1 XEUS_CPP_VERSION_MINOR) | ||
| list(GET VERSION_LIST 2 XEUS_CPP_VERSION_PATCH) |
There was a problem hiding this comment.
See the comment in xeus_cpp_config.hpp.on.
CMakeLists.txt
Outdated
| @@ -168,6 +175,9 @@ message("Configure kernels: ...") | |||
| configure_kernel("/share/jupyter/kernels/xcpp17/") | |||
| configure_kernel("/share/jupyter/kernels/xcpp20/") | |||
|
|
|||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/include/xeus-cpp/xeus_cpp_config.hpp.in" | |||
| "${CMAKE_CURRENT_SOURCE_DIR}/include/xeus-cpp/xeus_cpp_config.hpp") | |||
|
|
|||
There was a problem hiding this comment.
This should be removed (see the comment in xeus_cpp_config.hpp.in).
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
CMakeLists.txt
Outdated
| @@ -23,18 +23,28 @@ set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake;${CMAKE_MODULE_PATH}") | |||
|
|
|||
| set(XEUS_CPP_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/include) | |||
|
|
|||
| # xeus-cpp tag files | |||
| set(XEUS_CPP_DATA_DIR "share/xeus-cpp" CACHE STRING "xeus-cpp data directory") | |||
There was a problem hiding this comment.
Why do we need to put these in the cache?
There was a problem hiding this comment.
It was there in the previous code. CACHE just turns up in the cmake config tool as setting you can set. The initial commit for it was made by @mvassilev
There was a problem hiding this comment.
Ok. We can set them as normal variables.
|
clang-tidy review says "All clean, LGTM! 👍" |
| set(XCPP_TAGCONFS_DIR ${CMAKE_CURRENT_SOURCE_DIR}/etc/xeus-cpp/tags.d) | ||
|
|
||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/include/xeus-cpp/xeus_cpp_config.hpp.in" | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/include/xeus-cpp/xeus_cpp_config.hpp") |
There was a problem hiding this comment.
I think using target_compile_definition would be a better alternative than configuring hpp files from cmake.
However, let's merge it for now to have a reference to test against.
No description provided.