Skip to content

fix: Java version error; Add missing #! line#10

Merged
LukeSavefrogs merged 3 commits intoLukeSavefrogs:mainfrom
ZZy979:fix-java-version
May 14, 2024
Merged

fix: Java version error; Add missing #! line#10
LukeSavefrogs merged 3 commits intoLukeSavefrogs:mainfrom
ZZy979:fix-java-version

Conversation

@ZZy979
Copy link
Contributor

@ZZy979 ZZy979 commented May 5, 2024

This PR solves the following problems:

  1. Upgrade Java version from 8 to 11, since GitHub runner macos-latest no longer supports Java 8 (see Can't install temurin java 8 for macos actions/setup-java#625).

image

  1. Add missing #! line in the file ~/.local/bin/jython, which may cause an error when running jython by Python subprocess module.

image

Please review the changes. Thank you!

@ZZy979 ZZy979 force-pushed the fix-java-version branch from 92187d3 to 4473ea2 Compare May 5, 2024 14:01
@ZZy979
Copy link
Contributor Author

ZZy979 commented May 5, 2024

And this action seems not to work on Windows. When I just run "jython --version" using pwsh, I got the following error messages:

image

I printed the PATH env variable, and the added path seems incorrect.

image

How can I fix it?

@LukeSavefrogs
Copy link
Owner

LukeSavefrogs commented May 6, 2024

And this action seems not to work on Windows. When I just run "jython --version" using pwsh, I got the following error messages:

This is a known problem and is tracked by #8.

Feel free to investigate the problem 😄

@LukeSavefrogs
Copy link
Owner

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?

@ZZy979
Copy link
Contributor Author

ZZy979 commented May 6, 2024

This is a known problem and is tracked by #8.

Feel free to investigate the problem 😄

According to GitHub docs, different syntax should be used to add a system path on Windows PowerShell. I will try it later.

Did you test that upgrading the Java version does not break compatibility?

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.

@ZZy979
Copy link
Contributor Author

ZZy979 commented May 12, 2024

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?

@LukeSavefrogs
Copy link
Owner

I've updated the changes by switching to the zulu distribution, and tests have succeeded in macos-latest

This looks good!

As for the failing tests for Jython 2.0 and 2.1, I suggest you drop support for that old versions

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 😄

@LukeSavefrogs LukeSavefrogs merged commit 8365bb7 into LukeSavefrogs:main May 14, 2024
@ZZy979 ZZy979 deleted the fix-java-version branch May 14, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants