Initial merge with xeus-clang-repl (mini) #46

alexander-penev · 2024-03-16T15:37:55Z

No description provided.

src/xinterpreter.cpp

codecov-commenter · 2024-03-17T07:37:52Z

Codecov Report

Attention: Patch coverage is 53.70370% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 49.14%. Comparing base (2fcfd27) to head (8880063).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   49.29%   49.14%   -0.15%     
==========================================
  Files          15       15              
  Lines         779      643     -136     
==========================================
- Hits          384      316      -68     
+ Misses        395      327      -68

Files	Coverage Δ
src/xinterpreter.cpp	`66.45% <74.19%> (+10.24%)`	⬆️
src/xinspect.hpp	`57.14% <26.08%> (+0.42%)`	⬆️

... and 2 files with indirect coverage changes

Files	Coverage Δ
src/xinterpreter.cpp	`66.45% <74.19%> (+10.24%)`	⬆️
src/xinspect.hpp	`57.14% <26.08%> (+0.42%)`	⬆️

... and 2 files with indirect coverage changes

CMakeLists.txt

anutosh491 · 2024-03-22T08:01:44Z

src/xinterpreter.cpp

-        // Inject versions of printf and fprintf that output to std::cout
-        // and std::cerr (see implementation above).
-        inject_symbol("printf", llvm::pointerToJITTargetAddress(printf_jit), *m_interpreter);
-        inject_symbol("fprintf", llvm::pointerToJITTargetAddress(fprintf_jit), *m_interpreter);


Just curious to know if/why we don't need these functions anymore ?

We do have a better way to redirect i/o streams with CppInterOp. That will be part of a new PR as we discussed to do a minimal change with this one.

I think it is fine removing the injection of printf, but I think we should keep the C++ buffer redirection bits (as you did in this PR).

A file-based redirection won't give the same level of control as the streambuf implementation, and will also redirect things such as std::clog which are meant for the console.

Where does std::clog writes to? We only redirect file descriptors 1 and 2.

std::clog will print to stdout by default - so will be directed to the frontend.

Isn’t that what we should expect if a user type std::clog in a jupyter cell? If not we can make the redirection of std::clog go somewhere else.

test/test_interpreter.cpp

anutosh491 · 2024-03-22T08:33:26Z

I think this looks fine to start with (still need to go through the changes in CMakeLists.txt). Wrote a few comments. I was anyways unhappy with the special branching we had in process_code which was being used in execute_request_impl . That makes more sense now.

github-actions · 2024-03-22T10:25:17Z

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

SylvainCorlay · 2024-03-22T10:26:42Z

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

Could we disable the clang tidy "LGTM" comments?

mcbarton · 2024-03-22T10:35:34Z

@SylvainCorlay The following changes can be made to the README now that the feedstock has been updated (cannot seem to provide a link to the readme so are detailed below)
xeus-cpp has not been packaged for the mamba (or conda) package manager.
Since it is now available to for multiple platforms through conda-forge this can be changed.

mamba install notebook cmake cxx-compiler xeus-zmq nlohmann_json=3.11.2 cppzmq xtl jupyterlab clangdev=16 cpp-argparse pugixml doctest -c conda-forge
Needs to be changed to include CppInterOp, and possibly the restriction to clangdev=16

The dependencies section table needs to be updated to include the dependency on CppinterOp, and updated restriction of cpp-argparse for main.

SylvainCorlay · 2024-03-22T10:37:56Z

@SylvainCorlay The following changes can be made to the README now that the feedstock has been updated (cannot seem to provide a link to the readme so are detailed below)
xeus-cpp has not been packaged for the mamba (or conda) package manager.
Since it is now available to for multiple platforms through conda-forge this can be changed.

The mention that xeus-cpp has not been packaged should probably be changed.
The mention to cppinterop should be included in this PR I guess.

github-actions · 2024-03-22T11:32:39Z

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

CMakeLists.txt

vgvassilev · 2024-03-22T18:09:39Z

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

Could we disable the clang tidy "LGTM" comments?

We could but I find it useful for most PRs. That spares several clicks for the reviewers.

vgvassilev · 2024-03-25T12:02:47Z

@SylvainCorlay, can we make another review iteration?

github-actions · 2024-03-25T21:49:08Z

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

github-actions · 2024-03-26T05:31:17Z

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

github-actions · 2024-03-26T07:48:32Z

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

anutosh491 · 2024-03-26T12:39:34Z

environment-wasm-host.yml

  - xeus >=3.0.5,<4.0
  - xtl >=0.7,<0.8
-  - llvm =16.0.6
+  - CppInterOp
  - cpp-argparse


Should we pin this to <3.1 too ?

vgvassilev · 2024-04-02T19:00:40Z

@SylvainCorlay, can we move forward here? Is there something else we should improve as part of this pull request?

SylvainCorlay · 2024-04-02T23:29:51Z

Merged!

SylvainCorlay · 2024-04-02T23:47:55Z

share/jupyter/kernels/xcpp/kernel.json.in

@@ -1,9 +1,16 @@
 {
  "display_name": "cpp 14 (xcpp)",
+  "env": {
+      "PATH":"@XEUS_CPP_PATH@",
+      "LD_LIBRARY_PATH":"@XEUS_CPP_LD_LIBRARY_PATH@"


I merged, but I wonder why we need to set these up exactly, if it wasn't required before?

Hey @alexander-penev

I actually have a doubt along similar lines. I understand how something like resource dir can contribute and hence it is good to have it introduced in argv as done in the PR !

But I am not sure how PATH (especially LD_LIBRARY_PATH) has a role to play here. As pointed out by Sylvain, xeus-cling never needed these. Can it be removed ?

SylvainCorlay · 2024-04-02T23:51:42Z

CMakeLists.txt

+configure_file (
+    "${CMAKE_CURRENT_SOURCE_DIR}/share/jupyter/kernels/xcpp/kernel.json.in"
+    "${CMAKE_CURRENT_BINARY_DIR}/share/jupyter/kernels/xcpp/kernel.json"
+)


What is the benefit of this version compared to the previous one?

(trying to understand better because keeping cmake files of xeus-based kernels similar makes it easier to make changes across all kernels).

alexander-penev force-pushed the MergeXeusClangReplMini branch from 23fb2f4 to 78155cb Compare March 16, 2024 17:45

vgvassilev reviewed Mar 17, 2024

View reviewed changes

src/xinterpreter.cpp Outdated Show resolved Hide resolved

vgvassilev reviewed Mar 18, 2024

View reviewed changes

This comment was marked as resolved.

# to view

vgvassilev force-pushed the MergeXeusClangReplMini branch from a65e661 to 057483b Compare March 19, 2024 21:26

This comment was marked as resolved.

# to view

vgvassilev mentioned this pull request Mar 19, 2024

Add a test making sure we have set up STL properly #50

Closed

vgvassilev force-pushed the MergeXeusClangReplMini branch from a22d487 to 6c50ff7 Compare March 19, 2024 21:39

vgvassilev reviewed Mar 20, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

alexander-penev force-pushed the MergeXeusClangReplMini branch 2 times, most recently from b4c39de to 329a72b Compare March 21, 2024 01:58

This comment was marked as resolved.

# to view

alexander-penev force-pushed the MergeXeusClangReplMini branch from 60a8e7b to a3898f7 Compare March 21, 2024 02:32

vgvassilev force-pushed the MergeXeusClangReplMini branch from f739d95 to 0be6de3 Compare March 21, 2024 09:01

This comment was marked as resolved.

# to view

vgvassilev force-pushed the MergeXeusClangReplMini branch from 0be6de3 to e75bdc8 Compare March 21, 2024 09:09

compiler-research deleted a comment from github-actions bot Mar 21, 2024

anutosh491 reviewed Mar 22, 2024

View reviewed changes

test/test_interpreter.cpp Show resolved Hide resolved

Allow for cpp-argparse 3.0.

e5ec265

Update README.md

dd29feb

SylvainCorlay reviewed Mar 22, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

alexander-penev added 3 commits March 25, 2024 21:26

Undo new kernel configuration logic

0b77bb6

Undo some xcpp kernel settings

b59a960

Fix kernel config target path

07f3330

Fix new version of xeus-lite not working. Must be <2.0

15e0363

Fix resource path subst in kernel specs

8880063

anutosh491 reviewed Mar 26, 2024

View reviewed changes

JohanMabille approved these changes Mar 29, 2024

View reviewed changes

anutosh491 approved these changes Apr 2, 2024

View reviewed changes

SylvainCorlay approved these changes Apr 2, 2024

View reviewed changes

SylvainCorlay merged commit 06b171f into compiler-research:main Apr 2, 2024
7 checks passed

SylvainCorlay reviewed Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial merge with xeus-clang-repl (mini) #46

Initial merge with xeus-clang-repl (mini) #46

alexander-penev commented Mar 16, 2024

codecov-commenter commented Mar 17, 2024 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

anutosh491 Mar 22, 2024

vgvassilev Mar 22, 2024

SylvainCorlay Mar 22, 2024

vgvassilev Mar 22, 2024

SylvainCorlay Mar 22, 2024

vgvassilev Mar 22, 2024

anutosh491 commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

SylvainCorlay commented Mar 22, 2024

mcbarton commented Mar 22, 2024

SylvainCorlay commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

vgvassilev commented Mar 22, 2024

vgvassilev commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

github-actions bot commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

anutosh491 Mar 26, 2024 •

edited

Loading

vgvassilev commented Apr 2, 2024

SylvainCorlay commented Apr 2, 2024

SylvainCorlay Apr 2, 2024

anutosh491 Jan 6, 2025

SylvainCorlay Apr 2, 2024 •

edited

Loading

Initial merge with xeus-clang-repl (mini) #46

Initial merge with xeus-clang-repl (mini) #46

Conversation

alexander-penev commented Mar 16, 2024

codecov-commenter commented Mar 17, 2024 • edited Loading

Codecov Report

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anutosh491 commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

SylvainCorlay commented Mar 22, 2024

mcbarton commented Mar 22, 2024

SylvainCorlay commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

vgvassilev commented Mar 22, 2024

vgvassilev commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

github-actions bot commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

anutosh491 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

vgvassilev commented Apr 2, 2024

SylvainCorlay commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SylvainCorlay Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2024 •

edited

Loading

anutosh491 Mar 26, 2024 •

edited

Loading

SylvainCorlay Apr 2, 2024 •

edited

Loading