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

Skip computing resource dir and include paths for xeus-cpp-lite #183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Nov 19, 2024

While using xeus-cpp-lite ( or rather running clang-repl in the browser) , local paths to system includes or resource dirs don't make sense.

What should happen here is content out of

  1. Sysroot provided by emsdk (provided already)
  2. Resource dir hopefully provided by CppInterOp (in the works)

Should be collected in one location and then we need to load it using emscripten's --preload-file ( we can use this flag only once as it creates a .data file which comprises of all the headers/file we need)

So

  1. The resource dir path present in the kernel.json is of no use
  "argv": [
      "/Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/bin/xcpp",
      "-f",
      "{connection_file}",
      "-resource-dir", "/Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/lib/clang/19",
      "-I", "/Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/include",
      "-std=c++20"
  ],
  1. Using DetectResourceDir and DetectSystemCompilerIncludePaths can't be done as both depend on exec (marked as won't fix on emscripten)

  2. Init_includes() adds /Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/include to the cc1 command but this doesn't make sense either.

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Nov 19, 2024

The preloaded file would end up in FS/include or FS/include/wasm32-emscripten which are the only two places from where we can access the headers.

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.80%. Comparing base (a9ff145) to head (b3b51d9).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #183   +/-   ##
=======================================
  Coverage   80.80%   80.80%           
=======================================
  Files          19       19           
  Lines         974      974           
  Branches       93       93           
=======================================
  Hits          787      787           
  Misses        187      187           
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.39% <ø> (ø)
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.39% <ø> (ø)

Copy link
Contributor

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

Copy link
Contributor

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

@@ -357,7 +359,9 @@ __get_cxx_version ()

void interpreter::init_includes()
{
#ifndef EMSCRIPTEN
Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that hurt on emscripten? I'd think it's just some random non-existing folder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah random non existing folder that won't contribute to our solution :)

If you look at Anubhab's solution (https://wasmdemo.argentite.me/) or any other solutions that interest us (https://binji.github.io/wasm-clang/) we are only interested in two folders is what I can see

  1. include ( for /include/c++/v1)
  2. include/wasm32-emscripten (for /include/wasm32-emscripten/c++/v1)
// 1st link
"" -cc1 -triple wasm32-unknown-emscripten -emit-obj -mrelax-all -disable-free -clear-ast-before-backend -main-file-name "<<< inputs >>>" -mrelocation-model static -mframe-pointer=none -ffp-contract=on -fno-rounding-math -mconstructor-aliases -target-cpu generic -fvisibility=hidden -debugger-tuning=gdb -v -fcoverage-compilation-dir=/ -resource-dir lib/clang/17 -internal-isystem /include/wasm32-emscripten/c++/v1 -internal-isystem /include/c++/v1 -internal-isystem lib/clang/17/include -internal-isystem /include/wasm32-emscripten -internal-isystem /include -std=c++17 -fdeprecated-macro -fdebug-compilation-dir=/ -ferror-limit 19 -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fincremental-extensions -o "<<< inputs >>>.o" -x c++ "<<< inputs >>>"
clang -cc1 version 17.0.0 based upon LLVM 17.0.0git default target wasm32-unknown-emscripten
ignoring nonexistent directory "/include/wasm32-emscripten/c++/v1"
ignoring nonexistent directory "lib/clang/17/include"
ignoring nonexistent directory "/include/wasm32-emscripten"
#include "..." search starts here:
#include <...> search starts here:
/include/c++/v1
/include

// 2nd link
clang -cc1 -emit-obj -disable-free -isysroot / -internal-isystem /include/c++/v1 -internal-isystem /include -internal-isystem /lib/clang/8.0.1/include -ferror-limit 19 -fmessage-length 80 -fcolor-diagnostics -O2 -o test.o -x c++ test.cc

We eventually need to fetch anything we are interested in from one of the two folders and that is what --preload-file would do for us !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways the emscripten Ci would fail for now cause I am playing around with how the shared build for clangCppInteroP would work on emscripten-forge. As I mentioned on discord I wrote my thought on what should be done for cppinterop's emscripten build but I haven't heard anything back on it. Obviously we need to decide on a static build vs a shared build.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is does not having ifdefs around that code cause a failure on emscripten?

Copy link
Contributor

github-actions bot commented Dec 5, 2024

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

@anutosh491 anutosh491 mentioned this pull request Dec 9, 2024
4 tasks
Copy link
Contributor

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

@anutosh491
Copy link
Collaborator Author

Should be superseded by #199

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