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

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

Merged
merged 3 commits into from May 14, 2024

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
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
32 of 34 checks passed
@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.

None yet

2 participants