prioritize resource-dir provided by user over auto-detecting#791
prioritize resource-dir provided by user over auto-detecting#791Vipul-Cariappa merged 2 commits intocompiler-research:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #791 +/- ##
==========================================
+ Coverage 79.32% 79.40% +0.08%
==========================================
Files 11 11
Lines 3966 3987 +21
==========================================
+ Hits 3146 3166 +20
- Misses 820 821 +1
... and 2 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
clang-tidy review says "All clean, LGTM! 👍" |
lib/CppInterOp/CppInterOp.cpp
Outdated
| const std::vector<const char*>& GpuArgs /*={}*/) { | ||
| std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr); | ||
| std::string ResourceDir = MakeResourcesPath(); | ||
| std::string ResourceDir = parse_for(Args, "-resource-dir"); |
There was a problem hiding this comment.
Passing an empty resource dir should be wrong on the client side. I guess I do not understand this change.
There was a problem hiding this comment.
Among the Args passed to CreateInterpreter, I try to find the -resource-dir. If the passed value is valid, then I use it as it is. Otherwise, I try to detect it using existing methods.
Why is this required?
In Line 3400, we set the necessary library search location explicitly (Because by default, linker flags are ignored in repl).
In xeus-cpp setting, we (CppInterOp) cannot manually detect the correct resource. And the -resource-dir passed by xeus-cpp kernel is the correct location. Therefore, I am prioritising it over detecting it within CppInterOp.
Let me know if any other clarifications are required.
There was a problem hiding this comment.
Can you rename the function to extract resource dir argument and also put what you just said as a comment.
mcbarton
left a comment
mcbarton
left a comment
There was a problem hiding this comment.
I have tested this patch locally, and it was able to remove my hack from the openmp kernels PR. After this is in, it should just be a case of fixing the environment.yml file in my xeus-cpp PR to get the ci to pass without my hack. @Vipul-Cariappa Thanks for all your work solving this issue. I'd wait for @vgvassilev to approve this PR before taking it in though.
Vipul-Cariappa
force-pushed
the
dev/res-dir
branch
from
d6c5e8e to
0022d04
Compare
Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
No description provided.