-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: Java version error; Add missing #! line #10
Conversation
This is a known problem and is tracked by #8. Feel free to investigate the problem 😄 |
Thank you for your submission @ZZy979! I will review and eventually merge this PR as soon as I have spare time. Did you test that upgrading the Java version does not break compatibility? I would like not to since almost all companies I know that use Jython also are stuck to Java 8. I am keen to providing a way to specify Java version (defaulting to 8)... Do ytou know if it is possible to install legacy Jav 8 any other way? |
According to GitHub docs, different syntax should be used to add a system path on Windows PowerShell. I will try it later.
All CI workflows have succeeded (except for version 2.0 and 2.1, but I noticed they also failed in your last commit). As far as I know, only macos-latest doesn't support Java 8. Perhaps it's better to allow specifying Java version. |
I've updated the changes by switching to the zulu distribution, and tests have succeeded in macos-latest. Please take some time to have a look! As for the failing tests for Jython 2.0 and 2.1, I suggest you drop support for that old versions. Do you really need to keep the compatibility? |
This looks good!
As already said here i cannot drop support for Jython 2.1 since my action is used in some projects of mine and in the toolchain of my workplace production build system. I had to set for using windows runners instead of ubuntu (which are way faster) but it used to work perfectly, allowing me to discover bugs before they even went in production. I know that it's way too old but it's my work 😄 |
This PR solves the following problems:
Please review the changes. Thank you!