Skip to content

feat: offer basic support for fish for Java commands#2773

Merged
alexarchambault merged 2 commits intocoursier:mainfrom
ckipp01:fishJava
Jun 19, 2023
Merged

feat: offer basic support for fish for Java commands#2773
alexarchambault merged 2 commits intocoursier:mainfrom
ckipp01:fishJava

Conversation

@ckipp01
Copy link
Copy Markdown
Contributor

@ckipp01 ckipp01 commented May 29, 2023 •
edited
Loading

This PR offers some basic support for fish users that want to use coursier to manage their Java installations. Before this everything was .profileish based, which isn't supported by fish. It works a bit differently and fish users basically have the choice to only use the CLI when setting env variables with things like set -Ux JAVA_HOME <whatever> or then never have to update any files, or they can update their fish configs, which is typically located at XDG_CONFIG_HOME under fish/fish.config. Since we already had everything set up to add to files I went that route.

So this offers support for --env, for example:

❯ mill -i cli.run java --jvm 17 --env
[503/503] cli.run
set -x CS_FORMER_JAVA_HOME "$JAVA_HOME"
set -x JAVA_HOME "/Users/ckipp/Library/Caches/Coursier/arc/https/github.com/adoptium/temurin17-binaries/releases/download/jdk-17.0.7%252B7/OpenJDK17U-jdk_aarch64_mac_hotspot_17.0.7_7.tar.gz/jdk-17.0.7+7/Contents/Home"

It also supports the --disable flag. And finally it can now update your fish config when you're using --setup. For example:

❯ mill -i cli.run java --jvm 17 --setup
[503/503] cli.run
Checking if ~/.config/fish/config.fish need(s) updating.
Some shell configuration files were updated. It is recommended to close this terminal once the setup command is done, and open a new one for the changes to be taken into account.

And you'll then notice your fish config has the following:

# >>> JVM installed by coursier >>>
set -gx JAVA_HOME "/Users/ckipp/Library/Caches/Coursier/arc/https/github.com/adoptium/temurin17-binaries/releases/download/jdk-17.0.7%252B7/OpenJDK17U-jdk_aarch64_mac_hotspot_17.0.7_7.tar.gz/jdk-17.0.7+7/Contents/Home"
# <<< JVM installed by coursier <<<

Which correct sets your JAVA_HOME:

❯ echo $JAVA_HOME
/Users/ckipp/Library/Caches/Coursier/arc/https/github.com/adoptium/temurin17-binaries/releases/download/jdk-17.0.7%252B7/OpenJDK17U-jdk_aarch64_mac_hotspot_17.0.7_7.tar.gz/jdk-17.0.7+7/Contents/Home

All the existing behavior for non-fish shell users should remain the same.

@tgodzik
Copy link
Copy Markdown
Collaborator

tgodzik commented May 29, 2023

Hmm... coursier doesn't use case app yet? I remember there was a lot of things done there for completions etc. (not for fish though yet)

@ckipp01
Copy link
Copy Markdown
Contributor Author

ckipp01 commented May 29, 2023 •
edited
Loading

Hmm... coursier doesn't use case app yet? I remember there was a lot of things done there for completions etc. (not for fish though yet)

It does use it, but it doesn't have fish completions. Completions are also a whole other battle. I updated the description to make it clear, but this isn't offering completions, but rather just supporting being able to manage your java installation with fish.

I just got a new computer and decided to try to use coursier instead of sdkman to manage my java stuff, so these are basically the painpoints I'm hitting on 😆

@ckipp01 ckipp01 marked this pull request as ready for review May 29, 2023 12:38
alexarchambault
alexarchambault approved these changes May 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

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

Nice! If you feel like it, you might want to add non-regression tests to ProfileUpdaterTests

@ckipp01
Copy link
Copy Markdown
Contributor Author

ckipp01 commented May 30, 2023 •
edited
Loading

Nice! If you feel like it, you might want to add non-regression tests to ProfileUpdaterTests

Yes let me try to add something, because with the changed logic, the shell is now picked up in various places where it wasn't before and now on fish the tests actually don't pass anymore on a machine using fish, since all the others are testing for non-fish stuff.

Alright, added some tests! Which is good because it caused me to see that I actually had an issue 😄

alexarchambault
alexarchambault approved these changes Jun 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @ckipp01!

@alexarchambault alexarchambault merged commit 15e5ef1 into coursier:main Jun 19, 2023
@ckipp01 ckipp01 deleted the fishJava branch June 19, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexarchambault alexarchambault alexarchambault approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ckipp01 @tgodzik @alexarchambault