-
Notifications
You must be signed in to change notification settings - Fork 20
🐛 Fix exec_args for execvp #118
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
Conversation
Signed-off-by: Jason Montleon <[email protected]>
|
Instead of It now shows: |
aufi
left a comment
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.
The change looks OK, just linking PR with jdtls startup change replcing this script functionality konveyor/analyzer-lsp#749
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. |
Script Usage AnalysisI investigated whether this PR is still relevant given that analyzer-lsp#749 replaced the jdtls.py script with Go code in the java-external-provider. FindingsWhile the java-external-provider no longer uses this script, the script is still actively used by other Konveyor projects:
The container's default The BugThe bug is real. Python's exec_args = ["-Declipse.application=...", ...] # Missing java_executable
os.execvp(java_executable, exec_args) # ❌ Violates conventionThis PR correctly fixes it to include RecommendationMerge this PR. The script remains part of the published container image and is actively used by other projects. The fix is correct, approved, and all tests pass. |
|
@shawn-hurley - can you double check me before merging? |
shawn-hurley
left a comment
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 think this makes sense as well
I realize the goal is to get rid of this script, so no issue if we don't want to bother merging this. But the first arg for execvp is supposed to be the command itself.