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

Add utility for add library to environment variable using anchor #9

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

Conversation

Levi-Armstrong
Copy link
Contributor

This functionality is to support python wheels and is ported from this PR

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #9 (07f6770) into main (f3dbf6f) will decrease coverage by 5.67%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##              main       #9      +/-   ##
===========================================
- Coverage   100.00%   94.32%   -5.68%     
===========================================
  Files            2        2              
  Lines          144      194      +50     
===========================================
+ Hits           144      183      +39     
- Misses           0       11      +11     
Impacted Files Coverage Δ
...ader/include/boost_plugin_loader/plugin_loader.hpp 94.11% <0.00%> (-5.89%) ⬇️
src/boost_plugin_loader/src/utils.cpp 94.56% <0.00%> (-5.44%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

std::set<std::string> libraries_with_fullpath;
for (auto it = library_names.begin(); it != library_names.end();)
{
if (boost::filesystem::exists(*it))

Choose a reason for hiding this comment

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

This should also check to make sure the file is an absolute path using boost::filesystem::is_absolute()

setenv(search_libraries_env.c_str(), search_libraries_env_var_str.c_str(), 1);
setenv(search_paths_env.c_str(), search_paths_env_var_str.c_str(), 1);
setenv(search_libraries_env.c_str(), search_libraries_env_var_str.c_str(), 1);
setenv(search_paths_env.c_str(), search_paths_env_var_str.c_str(), 1);

Choose a reason for hiding this comment

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

We shouldn't be updating the search paths, we should use absolute paths for the libraries. Adding the entire path to be searched may have unpredictable results.

@Levi-Armstrong
Copy link
Contributor Author

@marip8 is working on splitting this up so the plugin loader only deals with absolute paths and there is a secondary class which is provided that does the searching and generation of the the list of library absolute paths. This way in Tesseract we can have a compile def which switches the way we locate plugins for the python wheel.

@Levi-Armstrong
Copy link
Contributor Author

@marip8 Have you taken a shot at the new interface?

@marip8
Copy link
Contributor

marip8 commented Jul 19, 2022

No I haven't had a chance to look into it much yet

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