Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
use installed JEP DLL rather than packaged DLL #50
use installed JEP DLL rather than packaged DLL #50
Changes from all commits
0f9d1de
c9daa62
5b308a5
5d9cbb7
ad8ca50
31bb1ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we assuming
python3
is available on all systems?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. can you give a scenario in which we expect Ghidrathon to work but python3 is not available? i can't immediately think of one, but i dont feel reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably don't need this with LibraryLocator, so lets not spend a lot of time answering this question unless that other strategy fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider using Jep's LibraryLocator?
LibraryLocator
should coverPYTHON_HOME
andVIRTUAL_ENV
lending less code for us to maintain. It won't, however, cover your third solution for resolving the shared Jep library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't consider it because i didn't notice it, thank you! i think i can replace most of this PR with a call to that routine 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dug into this some more and realized that
MainInterpreter
is coded to use theLibraryLocator
if theJep
library path isn't manually set...https://github.com/ninia/jep/blob/056ce9907f5ecbf2364df1ec55755404b2e8a947/src/main/java/jep/MainInterpreter.java#L124-L135. Essentially, if we don't do anything thenJep
attempts to find theJep
native library.I tested this locally in a Linux environment and it worked great with the caveat that the
LibraryLocator
is naive in that it chooses the firstJep
native library that it finds e.g. if you have two Python installs, both withJep
installed, then we don't have a choice (that I can find) as to which is selected. I don't think this is a major roadblock as it, so far, "just works" without additional code on our end and the multiple Python installs can be addressed by the user either by:Jep
installedI'm going to test this on Windows next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we collect all the paths first so that we can log out the status fully, then we actually set the config second.