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

Address resource dir config for kernels #172

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

Conversation

anutosh491
Copy link
Collaborator

Description

See https://discord.com/channels/1235591463472074924/1235591601523396680/1304415164753379441

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 Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.72%. Comparing base (dc14222) to head (fd0e463).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #172   +/-   ##
=======================================
  Coverage   80.72%   80.72%           
=======================================
  Files          19       19           
  Lines         970      970           
  Branches       93       93           
=======================================
  Hits          783      783           
  Misses        187      187           

@anutosh491
Copy link
Collaborator Author

Hmm weird, as can we seen through this commit (62980d0) using a new version of gcc (after updating cxx-compiler to 1.8.0 from 1.7.0) leads to few python tests failing. Let's debug this separately.

@anutosh491 anutosh491 changed the title Update cxx-compiler and address resource dir config Address resource dir config for kernels Nov 8, 2024
@anutosh491 anutosh491 force-pushed the fix_resource_dir_kernel_configuration branch from 62980d0 to d6d31bc Compare November 8, 2024 16:54
@anutosh491
Copy link
Collaborator Author

anutosh491 commented Nov 12, 2024

My understanding here is this.

For providing the resource dir

  1. We could either directly provide it through kernel.json or by manually while creating the interpreter.
  2. If the above is not done, we would need to use DetectResourceDir from CppInterOp.

But as can be seen in DetectResourceDir we have this (https://github.com/compiler-research/CppInterOp/blob/826be787bb542fe2394a425de78613a4da3eae15/lib/Interpreter/CppInterOp.cpp#L2760)

So as our resource dir is being provided out of clang (based from cxx-compiler) ... the latest that can be provided is version 17 but we use cppinterop compatible with latest clang version (so CLANG_VERSION_MAJOR_STRING is 19) and we don't satisfy the above condition.

Hence the ways to approach this is

  1. Maybe not use cxx-compiler and introduce a dependency on clang binary (so that we can get access to latest resource dir)
  2. Or fix kernel.json to reference the correct resource dir available out of cxx-compiler and provide the same for tests manually
        std::vector<const char*> Args = {"-v", "-resource-dir", XEUS_CPP_RESOURCE_DIR};
        xcpp::interpreter interpreter((int)Args.size(), Args.data());  

What has been done above is inclined towards the 2nd approach. I shall fix the tests in test_xinterpreter.cpp if we agree to continue with the same.

@mcbarton mcbarton force-pushed the fix_resource_dir_kernel_configuration branch from d6d31bc to fd0e463 Compare January 6, 2025 14:18
@anutosh491
Copy link
Collaborator Author

Not ready yet. Bad attempt to be fair. Converting to draft

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.

2 participants