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

Refactoring some utilities #153

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Jul 8, 2024

Few utilities that we use are already implemented and tested through xeus. So we can just get rid of them !

@anutosh491
Copy link
Collaborator Author

We should also be able to used the get_start_message function instead of the following

xeus-cpp/src/main.cpp

Lines 109 to 114 in 4745f72

std::clog << "Starting xcpp kernel...\n\n"
"If you want to connect to this kernel from an other client, just copy"
" and paste the following content inside of a `kernel.json` file. And then run for example:\n\n"
"# jupyter console --existing kernel.json\n\n"
"kernel.json\n```\n{\n"
" \"transport\": \""

Copy link
Contributor

github-actions bot commented Jul 8, 2024

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

@anutosh491
Copy link
Collaborator Author

Looked straightforward but for some reason the test_xcpp_kernel.py test fails for some reason. Need to have a look into it

@anutosh491
Copy link
Collaborator Author

@vgvassilev @mcbarton I think this might have something to do with the patches in CppInterOp (y'all could help by confirming) cause through backtrace we get

error: error reading '/Users/runner/micromamba/envs/xeus-cpp/include': Is a directory
1 error generated.
Error: signal 11:
0   libxeus-cpp.0.5.0dev.dylib          0x000000010d492fbe _ZN4xcpp7handlerEi + 46
1   libsystem_platform.dylib            0x00007ff809df75ed _sigtramp + 29
2   ???                                 0x0000000000000004 0x0 + 4
3   libclangCppInterOp.18.1.dylib       0x0000000117a94620 _ZN5clang17IncrementalParserC2ERNS_11InterpreterENSt3__110unique_ptrINS_16CompilerInstanceENS3_14default_deleteIS5_EEEERN4llvm11LLVMContextERNS9_5ErrorE + 208
4   libclangCppInterOp.18.1.dylib       0x0000000117a97cca _ZN5clang11InterpreterC2ENSt3__110unique_ptrINS_16CompilerInstanceENS1_14default_deleteIS3_EEEERN4llvm5ErrorE + 346
5   libclangCppInterOp.18.1.dylib       0x0000000117a97f44 _ZN5clang11Interpreter6createENSt3__110unique_ptrINS_16CompilerInstanceENS1_14default_deleteIS3_EEEE + 84
6   libclangCppInterOp.18.1.dylib       0x0000000117a78e42 _ZN6compat22createClangInterpreterERNSt3__16vectorIPKcNS0_9allocatorIS3_EEEE + 386
7   libclangCppInterOp.18.1.dylib       0x0000000117a78a73 _ZN3Cpp11InterpreterC2EiPKPKcS2_RKNSt3__16vectorINS5_10shared_ptrIN5clang19ModuleFileExtensionEEENS5_9allocatorISA_EEEEPvb + 883
8   libclangCppInterOp.18.1.dylib       0x0000000117a6d3c9 CreateInterpreter + 1833
9   libxeus-cpp.0.5.0dev.dylib          0x000000010d456c44 _Z17createInterpreterRKNSt3__16vectorIPKcNS_9allocatorIS2_EEEE + 1492

@anutosh491
Copy link
Collaborator Author

Caught the bug here. Sadly I was looking at all places except the function I had changed (the stack trace didn't help and confused me even more).

It's an error with how xeus deals with extract_filename function, so have to fix that upstream.

@vgvassilev
Copy link
Contributor

Why the nightly builds succeed then?

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.55%. Comparing base (f678e0d) to head (0f275bf).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
- Coverage   81.83%   81.55%   -0.29%     
==========================================
  Files          19       19              
  Lines         837      824      -13     
  Branches       91       86       -5     
==========================================
- Hits          685      672      -13     
  Misses        152      152              
Files with missing lines Coverage Δ
src/main.cpp 42.85% <100.00%> (ø)
src/xutils.cpp 72.41% <ø> (-8.54%) ⬇️
Files with missing lines Coverage Δ
src/main.cpp 42.85% <100.00%> (ø)
src/xutils.cpp 72.41% <ø> (-8.54%) ⬇️

Copy link
Contributor

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

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants