Add PANDA_PLUGIN_PATH#1408
Conversation
jamcleod
left a comment
jamcleod
left a comment
There was a problem hiding this comment.
Overall design question: using some combination of PANDA_PATH and PANDA_PLUGIN_DIR depending on needs has been the way of handling this usecase in the past. Is there a reason we're making a new variable with overlapping usecases rather than fixing the previous implementation?
The main differences I see here are:
- supports providing multiple paths (beneficial, but I don't think requires adding a new environment variable)
- supports absolute paths (the current code does support this, albeit inconsistently and definitely has bugs for usecases I haven't personally considered. however to me this would be a reason to fix the current code rather than)
This is mostly coming from a place of wanting to avoid having 5 different ways of doing this, each with their own little bugs, with documentation being unclear which one to use. It's also possible that incrementally deprecating the old system is the correct move, all up for consideration.
not saying these are reasons not to merge, just questions worth considering before merging. also be sure to notify me when the design has been ironed out so I can ensure panda-rs also receives a corresponding update.
| plugin_name_ffi = self.ffi.new("char[]", bytes(plugin_name, "utf-8")) | ||
| plugin_path_ffi = self.libpanda.panda_plugin_path(plugin_name_ffi) | ||
| plugin_path = self.ffi.string(plugin_path_ffi).decode("utf-8") | ||
| # TODO: self.libpanda.g_free(plugin_path_ffi) |
There was a problem hiding this comment.
I'm pretty sure this is a memory leak without the free, but CFFI doesn't seem to find the g_free() function, which is weird because other parts of the code find g_malloc0(). I don't know what's going on here.
| else: | ||
| raise ValueError(f"Could not find plugin path. Build dir={build_dir}") | ||
| return self.plugin_path | ||
| def get_plugin_path(self, plugin_name): |
There was a problem hiding this comment.
this doesn't seem to cover all the cases from before, was the /usr/local/bin special-casing redundant? it also doesn't appear to perform error propagation/checking, so this is a breaking change (as catching ValueError to handle missing plugins isn't possible)
| gchar *plugin_path = attempt_normalize_path(g_strdup_printf( | ||
| "%s/%s/%s", *dir, | ||
| TARGET_NAME, name_formatted)); | ||
| if (TRUE == g_file_test(plugin_path, G_FILE_TEST_EXISTS)) { |
There was a problem hiding this comment.
minor nit but don't think you need to actually check against glibc TRUE here. just checking for non-zero return is a superset of checking for glib TRUE, so there's no real reason to write in this style.
There was a problem hiding this comment.
I can change it but all the other tests in the function use the same style so it would feel inconsistent.
Mostly because I'm afraid of breaking backwards compatibility and I don't know how all the different cases apply to how PANDA is used in practice. Do you think it would be better to make |
2 participants
Add a colon-separated
PANDA_PLUGIN_PATHenvironment variable for specifying plugin directories similar toPATH,PYTHONPATH, andLD_LIBRARY_PATH.