Discussing -resource-dir flag while creating interpreter

[anutosh491]

1. Native case

• We always will be providing it from kernel.jsoN. So this block looks redundant
  

  xeus-cpp/src/xinterpreter.cpp

  Lines 34 to 41 in 48767d6

  	if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) {
  	return s == "-resource-dir";}) == ExtraArgs.end()) {
  	std::string resource_dir = Cpp::DetectResourceDir();
  	if (resource_dir.empty())
  	std::cerr << "Failed to detect the resource-dir\n";
  	ClangArgs.push_back("-resource-dir");
  	ClangArgs.push_back(resource_dir.c_str());
  	}

• Even if it is not redundant, it would fail (cause there's an implementation logical error I think)

std::string resource_dir = Cpp::DetectResourceDir();  // we don't pass anything here 

// function from cppinterop is framed like this 

  std::string DetectResourceDir(const char* ClangBinaryName /* = clang */) {
    std::string cmd = std::string(ClangBinaryName) + " -print-resource-dir";
    std::vector<std::string> outs;
    exec(cmd.c_str(), outs);
    if (outs.empty() || outs.size() > 1)
      return "";
    
   ......

Here ClangBinaryName would end up being empty as we're not passing anything (probably should pass "clang") and hence this would return an empty string resulting in std::cerr << "Failed to detect the resource-dir\n"; this error being executed.

• After this I see more duplication in how cppinterop creates the interpreter

I see

  TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
                              const std::vector<const char*>& GpuArgs /*={}*/) {
    std::string MainExecutableName =
      sys::fs::getMainExecutable(nullptr, nullptr);
    std::string ResourceDir = MakeResourcesPath();
    std::vector<const char *> ClingArgv = {"-resource-dir", ResourceDir.c_str(),
                                           "-std=c++14"};
    ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());
........
    ClingArgv.insert(ClingArgv.end(), Args.begin(), Args.end());
......

Shouldn't we prefer having (possibly also checking if the -std=... flag is also covered already)

std::vector<const char *> ClingArgv = {"-std=c++14"};

if (std::find_if(Args.begin(), Args.end(), [](const char* s) {
      return std::string(s) == "-resource-dir";}) == Args.end()) {
    std::string ResourceDir = MakeResourcesPath();
    ClingArgv.insert(ClingArgv.begin(), "-resource-dir");
    ClingArgv.insert(ClingArgv.begin() + 1, ResourceDir.c_str());
}

[anutosh491]

2. LITE case

• For the lite use case, we currently see this (coming from here again)

"" -cc1 -triple wasm32-unknown-emscripten .....-resource-dir /Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/lib/clang/19 ..... -o "<<< inputs >>>.o" -x c++ "<<< inputs >>>"

and the following

ignoring nonexistent directory "/Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/lib/clang/19/include"

The above path looks entirely misplaced in a browser/JupyterLite environment.

• How resource dir for lite would work is

1. CppInterOp would first provide a resource dir
2. We would then preload it here
   

   xeus-cpp/CMakeLists.txt

   Lines 446 to 449 in 48767d6

   	target_link_options(xcpp
   	PUBLIC "SHELL: --preload-file ${SYSROOT_PATH}/include@/include"
   	PUBLIC "SHELL: --preload-file ${XEUS_CPP_DATA_DIR}@/share/xeus-cpp"
   	PUBLIC "SHELL: --preload-file ${XEUS_CPP_CONF_DIR}@/etc/xeus-cpp"

3. We would use PUBLIC "SHELL: --preload-file ${RESOURCE_DIR}/include@/include"
4. And finally pass the flag here
   

   xeus-cpp/src/xinterpreter.cpp

   Lines 29 to 33 in 48767d6

   	void* createInterpreter(const Args &ExtraArgs = {}) {
   	Args ClangArgs = {/*"-xc++"*/"-v"}; // ? {"-Xclang", "-emit-llvm-only", "-Xclang", "-diagnostic-log-file", "-Xclang", "-", "-xc++"};
   	#ifdef EMSCRIPTEN
   	ClangArgs.push_back("-std=c++20");
   	#else

using

ClangArgs.push_back("-resource-dir ");
ClangArgs.push_back("/include ");

cause everything would be preloaded into the jupyterlite's /inlcude dir as we wrote above.

5. This again makes the following redundant

xeus-cpp/src/xinterpreter.cpp

Lines 34 to 41 in 48767d6

	if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) {
	return s == "-resource-dir";}) == ExtraArgs.end()) {
	std::string resource_dir = Cpp::DetectResourceDir();
	if (resource_dir.empty())
	std::cerr << "Failed to detect the resource-dir\n";
	ClangArgs.push_back("-resource-dir");
	ClangArgs.push_back(resource_dir.c_str());
	}

6. And again by now

i) We would have the correct -resource-dir flags so we can stop the duplication already as I wrote earlier in cppinterop
ii) If we don't handle the duplication, we anyways can't get MakeResourcesPath work because of the sys calls which won't work with emscripten. So in that case, we just add

#ifndef EMSCRIPTEN
    std::string MainExecutableName =
      sys::fs::getMainExecutable(nullptr, nullptr);
    std::string ResourceDir = MakeResourcesPath();
    std::vector<const char *> ClingArgv = {"-resource-dir", ResourceDir.c_str(),
                                           "-std=c++14"};
    ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());
#else

Cause a path like /Users/anutosh491/micromamba/envs/xeus-cpp-wasm-host/lib/clang/19 can not be put to use and we would also have the correct path by then.

[anutosh491]

What can be done here (these are common things that both the native & lite cases would end up needing)

1. Maybe get rid of the redundant block (or fix it if we think its not redundant)
2. Fix the duplication in cppinterop

[kr-2003]

Here ClangBinaryName would end up being empty as we're not passing anything (probably should pass "clang") and hence this would return an empty string resulting in std::cerr << "Failed to detect the resource-dir\n"; this error being executed.

This is not the case. Default argument in Cpp:: DetectResourceDir is clang. So, if we even don't pass any argument, clang -print-resource-dir will be executed.

Inside CppInterOp.h(link):

CPPINTEROP_API std::string
DetectResourceDir(const char* ClangBinaryName = "clang");

[kr-2003]

Shouldn't we prefer having (possibly also checking if the -std=... flag is also covered already)

std::vector<const char *> ClingArgv = {"-std=c++14"};

if (std::find_if(Args.begin(), Args.end(), [](const char* s) {
      return std::string(s) == "-resource-dir";}) == Args.end()) {
    std::string ResourceDir = MakeResourcesPath();
    ClingArgv.insert(ClingArgv.begin(), "-resource-dir");
    ClingArgv.insert(ClingArgv.begin() + 1, ResourceDir.c_str());
}

Here you proposed to check for -resource-dir in the args(to check whether resource dir flag is even present or not).
But if here in this code:

 if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) { 
   return s == "-resource-dir";}) == ExtraArgs.end()) { 
   std::string resource_dir = Cpp::DetectResourceDir(); 
   if (resource_dir.empty()) 
     std::cerr << "Failed to detect the resource-dir\n"; 
   ClangArgs.push_back("-resource-dir"); 
   ClangArgs.push_back(resource_dir.c_str()); 
 } 

It is pushing the flag in ClangArgs(if it does not find -resource-dir in ExtraArgs) and then the CreateInterpreter(ClangArgs) is called. So, basically no need to check again.

[kr-2003]

Yeah, I do agree that

 if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) { 
   return s == "-resource-dir";}) == ExtraArgs.end()) { 
   std::string resource_dir = Cpp::DetectResourceDir(); 
   if (resource_dir.empty()) 
     std::cerr << "Failed to detect the resource-dir\n"; 
   ClangArgs.push_back("-resource-dir"); 
   ClangArgs.push_back(resource_dir.c_str()); 
 } 

this is redundant. Because, if ExtraArgs always will have -resource-dir because it is taken from kernel.json.

[notanaveragelifter]

This fixes the issue by:

1. Removing the redundant -resource-dir check (already handled in cppinterop)
2. Ensuring correct path for Emscripten builds

#ifdef EMSCRIPTEN
    ClangArgs.push_back("-resource-dir");
    ClangArgs.push_back("/include");
#endif

[anutosh491]

@kr-2003 is anything left here to be addressed ?

[kr-2003]

@kr-2003 is anything left here to be addressed ?

Yeah, there is nothing left ig. We agreed that we need the -resource-dir checking code. Even though we always take args from kernel.json, it doesn't mean that will always be the case. If the user tampers with the kernel.json, then this code will be helpful in that case.