-
Notifications
You must be signed in to change notification settings - Fork 36
Make XEUS_CPP_RESOURCE_DIR a user configurable option #312
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
Make XEUS_CPP_RESOURCE_DIR a user configurable option #312
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
=======================================
Coverage 81.78% 81.78%
=======================================
Files 20 20
Lines 950 950
Branches 87 87
=======================================
Hits 777 777
Misses 173 173 🚀 New features to boost your workflow:
|
Will change this to be clang resource dir later today |
19ab695
to
5a7d6c8
Compare
Hey @mcbarton I've made the change. Let me know if you have any doubts. This basically confirms that XEUS_CPP_RESOURCE_DIR is "prefix/lib/clang/cppinterop_major_version" for all cases if not provided by the user. And for the wasm cases, we should preload these headers to This should be good enough to get the cppinterop ci fixed (where we just reference XEUS_CPP_RESOURCE_DIR while building) |
Thanks for checking this with the relevant pr on cppinterop. I think we're good to go ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confident on the changes on native and wasm kernels here.
As for native, we see this in the kernelspec
"argv": [
"/Users/anutosh491/micromamba/envs/xeus-cpp/bin/xcpp",
"-f",
"{connection_file}",
"-resource-dir", "/Users/anutosh491/micromamba/envs/xeus-cpp/lib/clang/20",
"-I", "/Users/anutosh491/micromamba/envs/xeus-cpp/include",
"-std=c++23"
]
And for wasm we see this in the kernel spec
"argv": [
"/Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/bin/xcpp",
"-resource-dir", "/lib/clang/20",
"-std=c++23"
]
Which is the exact place we are preloading the resource dir headers in the VFS (and should be accessible inside /lib/clang/20/include as shown below)
Description
Please include a summary of changes, motivation and context for this PR.
The llvm resource directory is now unconditionally assumed to be installed in the CMAKE_INSTALL_PREFIX . This has caused problems in CppInterOp where we do not install it in this location. Therefore this PR makes the clang resource directory an option that can be provided by the user, and fallsback to assuming its in the install prefix if not defined.
Fixes # (issue)
Type of change
Please tick all options which are relevant.