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

Allow jdtls.py to be dynamically imported #3386

Merged
merged 2 commits into from
Feb 19, 2025
Merged

Conversation

Trenek
Copy link
Contributor

@Trenek Trenek commented Feb 14, 2025

Previous jdtls script didn't import jdtls.py properly, this is a problem I happened to have.
image
The proposed change solves this problem entirely by importing jdtls.py module dynamically

Signed-off-by: Trenek <[email protected]>
@eclipse-ls-bot
Copy link
Contributor

Can one of the admins verify this patch?

@rgrunber
Copy link
Contributor

rgrunber commented Feb 18, 2025

Would you be able to set PYTHONPATH in your shell to point to the directory that contains jdtls.py. After that you would be able to run jdtls wherever you place it, without the need to dynamically import the module in the script.

@Trenek
Copy link
Contributor Author

Trenek commented Feb 19, 2025

Just checked it out and apparently addition of PYTHONPATH doesn't change a thing
image
Even if there are some other fixes, wouldn't a change to a script in order to make it more user-friendly (and especially java-dev-with-no-knowledge-about-python-friendly), this way or the other, be beneficial in the long run? 🤔

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Ok, had to ask since even https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly mentions this approach should be a last resort.

@rgrunber
Copy link
Contributor

I keep forgetting this becomes an issue for contributors outside Eclipse, but would you be able to sign the ECA : https://www.eclipse.org/legal/eca/ ? If you really can't I can try double checking if there's any criteria where I can bypass, but I'm pretty sure it needs to be signed.

@Trenek
Copy link
Contributor Author

Trenek commented Feb 19, 2025

Oh, sure, I actually already did, but by clicking commit on github it automatically gave my other e-mail lol

@Trenek
Copy link
Contributor Author

Trenek commented Feb 19, 2025

ok, I did change the commit author to my other email, now everything seems fine ^^

@rgrunber rgrunber added this to the End February 2025 milestone Feb 19, 2025
@rgrunber rgrunber merged commit eb8536d into eclipse-jdtls:master Feb 19, 2025
6 checks passed
@rgrunber rgrunber changed the title python script fix Allow jdtls.py to be dynamically imported Feb 19, 2025
@rgrunber
Copy link
Contributor

Thanks for taking the time to do this!

@rgrunber
Copy link
Contributor

rgrunber commented Feb 19, 2025

@Trenek out of curiosity, would you be able to share how you were setting up JDT-LS in order to run into this issue. Were jdtls.py and jdtls really kept in the same folder ? Where did you download it from ? The part that's confusing me is, this fix basically just dynamically loads jdt.ls.py from the same directory as the jdtls script. But this should already have worked without the change, since the script's directory should be on python's module path.

For example, if I just create a simple python script to output the module path :

$ cat test.py 
#! /usr/bin/python
import sys
print(sys.path)

executing it from any location, will always include the script's directory on the module path :

$ /tmp/foo/test.py 
['/tmp/foo', '/usr/lib64/python313.zip', '/usr/lib64/python3.13', '/usr/lib64/python3.13/lib-dynload', '/usr/lib64/python3.13/site-packages', '/usr/lib/python3.13/site-packages']

@Trenek
Copy link
Contributor Author

Trenek commented Feb 19, 2025

I downloaded jdtls from here, jdt-language-server-latest.tar.gz on a pretty much fresh Windows 10 installation and then downloaded python-3.13.2-embed-amd64.zip
After downloading python and jdtls I just unzipped those and created an environment variables

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants