System Java now works on macOS #142
Conversation
241a906 to
5ef46c0
Compare
Fixes halcyon#132 Added special case for macOS in all 4 set-java-home scripts. Also created tests to validate this. Unfortunately xonsh does not seem to have a way to exit with other status then 0, so this will not report errors (yet). Updated the README.md to contain information on how to use the set-java-home scripts. Cloned mstksg/get-package and released delgurth/get-package@v3 because it was using linuxbrew on Ubuntu and because I needed the apt-update (not released by mstksg) and -y flag in the apt-get install (when running github action on https://github.com/nektos/act) added .editorconfig because of mixed indent styles added .gitignore to ignore Intellij directory asdf/asdf.sh needs $PWD in front as soon as you change directories added tests for JAVA_HOME setting added tests for macOS JAVA_HOME setting in case version is set to system Unfortunately the tests are not exactly as it would be on a system because of the way github actions spawn shells. Have to call the bash function _asdf_java_prompt_command and the fish & zsh function asdf_update_java_home to actually update the JAVA_HOME.
…w fail with an error if something is wrong. Also suppressed output of the grep commands, we are only interested in the command result status, not in the output.
5ef46c0 to
a0f78f5
Compare
…. Fixed that as well.
|
LGTM. @joschi thoughts? |
|
@joschi did you have time to look at this pull-request? |
| @@ -16,10 +16,10 @@ jobs: | |||
| TERM: dumb | |||
| steps: | |||
| - uses: actions/checkout@v2 | |||
| - uses: mstksg/get-package@v1 | |||
| - uses: delgurth/get-package@v3 | |||
There was a problem hiding this comment.
Any specific reason the replace the GitHub Action here?
There was a problem hiding this comment.
Yes, there was a specific reason, v1 installed packages twice on newer Ubuntu images, once via brew and once via the normal apt-get. mstksg/get-package#5
| @@ -0,0 +1,5 @@ | |||
| [*] | |||
| indent_style = space | |||
| indent_size = 2 | |||
There was a problem hiding this comment.
@delgurth Could you please either update the indent_size in .editorconfig to match the existing code or remove the file altogether, and revert the whitespace related changes in this PR?
There was a problem hiding this comment.
I'll remove all formatting changes. I've tried to align it, but if I recall correctly the formatting was different in different parts of the codebase.
| echo "java_macos_integration_enable = yes" > "${ASDF_CONFIG_FILE}" | ||
| . $PWD/asdf/asdf.sh | ||
| asdf plugin add java "$GITHUB_WORKSPACE" | ||
| asdf install java adoptopenjdk-8.0.252+9.1.openj9-0.20.0 |
There was a problem hiding this comment.
After the changes in #140, this (and all subsequent usages of this identifier in this file) should probably be adoptopenjdk-openj9-8.0.252+9.1.openj9-0.20.0 or adoptopenjdk-8.0.252+9.1.
There was a problem hiding this comment.
Will see what is needed now to get things to work again properly. Been a long time since I last worked on this...
| @@ -0,0 +1 @@ | |||
| .idea/ | |||
There was a problem hiding this comment.
Could we remove this file again?
There was a problem hiding this comment.
I've added this since I'm working from IntelliJ IDEA. This way I prevent my IDEA setup to be checked in, which I think is preferable?
There was a problem hiding this comment.
I think this should be in your own global .gitignore file instead of this repository. 😉
| @@ -11,46 +11,51 @@ | |||
| - [unzip](http://infozip.sourceforge.net/UnZip.html) | |||
| - [jq](https://stedolan.github.io/jq/) (only for updating the release data) | |||
|
|
|||
| _Besides bash (the shell used by the maintainer) this plugin **should** work on Fish, Zsh and Xonsh_ | |||
There was a problem hiding this comment.
The tests in the GitHub workflows should show that it's working. 😉
There was a problem hiding this comment.
True, but I wanted to make clear what was guaranteed. Getting the Xonsh part to work properly took me quite some time, which annoyed me a bit. That's what made me add this line.
|
Why this PR is still open since 2021-04-05? |
|
It's still open because the issues joschi commented on were not resolved. |
|
I'll try to redo the work, since it's giving quite some conflicts now. |
|
@delgurth if You need help, please let me know, I would be glad to help out |
Fixes #132
Added special case for macOS in all 4 set-java-home scripts. Also created tests to validate this.
Updated the README.md to contain information on how to use the set-java-home scripts.
Cloned mstksg/get-package and released delgurth/get-package@v3 because it was using linuxbrew on Ubuntu and because I needed the apt-update (not released by mstksg) and -y flag in the apt-get install (when running github action on https://github.com/nektos/act). I've created an issue in the mstksg repository about this.
added .editorconfig because of mixed indent styles
added .gitignore to ignore Intellij directory
asdf/asdf.sh needs $PWD in front as soon as you change directories
added tests for JAVA_HOME setting
added tests for macOS JAVA_HOME setting in case version is set to system
Unfortunately the tests are not exactly as it would be on a system because of the way github actions spawn shells. Have to call the bash function _asdf_java_prompt_command and the fish & zsh function asdf_update_java_home to actually update the JAVA_HOME.