Skip to content

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

Merged
merged 4 commits into from
May 27, 2025

Conversation

mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented May 25, 2025

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.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.78%. Comparing base (51aeae3) to head (765e90d).

Additional details and impacted files

Impacted file tree graph

@@           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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vgvassilev vgvassilev requested a review from anutosh491 May 25, 2025 18:22
@mcbarton
Copy link
Collaborator Author

Will change this to be clang resource dir later today

@mcbarton mcbarton changed the title Make LLVM_RESOURCE_DIR an option that can be provided by the user Make CLANG_RESOURCE_DIR an option that can be provided by the user May 26, 2025
@mcbarton mcbarton force-pushed the Fix-resource-directory branch from 19ab695 to 5a7d6c8 Compare May 26, 2025 08:24
@anutosh491 anutosh491 changed the title Make CLANG_RESOURCE_DIR an option that can be provided by the user Make XEUS_CPP_RESOURCE_DIR a user configurable option May 26, 2025
@anutosh491
Copy link
Collaborator

anutosh491 commented May 26, 2025

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 /lib/clang/cppinterop_major_version from where they can be accessed.

This should be good enough to get the cppinterop ci fixed (where we just reference XEUS_CPP_RESOURCE_DIR while building)

@anutosh491
Copy link
Collaborator

Thanks for checking this with the relevant pr on cppinterop.

I think we're good to go !

Copy link
Collaborator

@anutosh491 anutosh491 left a 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)

image

@anutosh491 anutosh491 merged commit fa7f872 into compiler-research:main May 27, 2025
14 checks passed
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