-
Notifications
You must be signed in to change notification settings - Fork 282
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
add support for macos integration #858
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #858 +/- ##
==========================================
- Coverage 87.62% 87.47% -0.16%
==========================================
Files 132 132
Lines 11552 11576 +24
==========================================
+ Hits 10123 10126 +3
- Misses 1429 1450 +21
☔ View full report in Codecov by Sentry. |
thanks for taking this on, I have been looking at this over the past couple of weeks but have't found the time to work on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will try to test this myself probably tomorrow
docs/java.md
Outdated
|
||
```sh | ||
sudo mkdir /Library/Java/JavaVirtualMachines/openjdk-20 | ||
ln -s ~/.local/share/rtx/installs/java/openjdk-20/Contents /Library/Java/JavaVirtualMachines/openjdk-20/Contents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs sudo as well
Great. Thanks @jdx. Ran some manual tests with almost all distributions. Iberica does not ship with the required files and Zulu has the files but they're located in a different sub-folder (e.g. zulu-11.jdk). Added an additional comment in the docs about that. |
src/plugins/core/java.rs
Outdated
&tv.install_path().join("Contents").join("Home"), | ||
)?; | ||
info!( | ||
"To enable macOS integration, run the following commands: \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, the output is not optimal and mixes even different output types (progress message java -version
).
you're probably right, package installation folders usually end in .jdk
even though it also seems to work fine without.
src/plugins/core/java.rs
Outdated
&tv.install_path().join("Contents").join("Home"), | ||
)?; | ||
info!( | ||
"To enable macOS integration, run the following commands:\n\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rust supports linebreaks inside of strings so you can just remove the end of these lines entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried that, it interpreted the spaces in front of the commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is first rough draft. Which works with with
sudo
, what i currently don't like yet is that usingsudo
installs the JDK withroot
as the owner which currently fails with an unspecific error (since installation is removed before the symlink remove is attempted). Having root as owner potentially leads to other issues as well.