Skip to content

Use the current user's SDKMAN installation #718

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use the current user's SDKMAN installation #718

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34cf7f1
Reset SDKMAN_DIR if it has wrong owner
reitzig Nov 6, 2019
bb48546
Introduce sdkman_owner variable for clarity
reitzig Nov 6, 2019
19f161f
Prefer double-bracket test.
reitzig Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/bash/sdkman-init.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ if [ -z "$SDKMAN_CANDIDATES_API" ]; then
export SDKMAN_CANDIDATES_API="@SDKMAN_CANDIDATES_API@"
fi

if [ -z "$SDKMAN_DIR" ]; then
if [ -z "$SDKMAN_DIR" ] || [ $(ls -ld "${SDKMAN_DIR}" | awk '{ print $3 }') != $(whoami) ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Although I do see the point, the implementation does feel a bit hacky to me.
Can we assign this expression to a clearly named variable? Can we unset said variable at the end of the script? Can we collapse the two conditional blocks into one?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, and of course, please refrain from using awk as this forces a hard dependency that will often not be met on very basic systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, and of course, please refrain from using awk as this forces a hard dependency that will often not be met on very basic systems.

Which systems are that? The smallest I know comes with (an) awk:

$ docker run --rm alpine awk -version
BusyBox v1.30.1 (2019-06-12 17:51:55 UTC) multi-call binary.

Of course, BusyBox binaries are quite often not compatible with GNU ones... :/

That said, which dependencies are you comfortable with? For instance, I used to use the innocuous stat but it turns out that macOS and Ubuntu come with incompatible versions (FreeBSD-ish vs GNU-ish). ls + awk was the best I could come up with -- I'm open for suggestions!

Copy link
Member

Choose a reason for hiding this comment

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

The cut command is preferable as it is packaged as part of gnu coreutils. You could try something like this:

ls -ld "${SDKMAN_DIR}" | cut -d" " -f3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I do see the point, the implementation does feel a bit hacky to me.

Do you have a proposal? I felt it's a pretty straight-forward implementation for "do we point at this user's installation?". Of course, it will fail with system installations, but IIRC that's not a use-case you support?

Can we assign this expression to a clearly named variable? Can we unset said variable at the end of the script?

Sure!

Can we collapse the two conditional blocks into one?

What do you mean? [ -z ... ] || [ ... ] into [ ... ]? No, I don't think so; [ 1 -eq 1 || 1 -eq 2 ] doesn't parse. [[ 1 -eq 1 || 1 -eq 2 ]] would work, but you use single brackets everywhere else?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, yes I do think so. We use double square brackets throughout the sdkman codebase when dealing with complex expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cut command is preferable as it is packaged as part of gnu coreutils. You could try something like this:

ls -ld "${SDKMAN_DIR}" | cut -d" " -f3

This does fail on the current Alpine (BusyBox v1.30.1). :/

Then, maybe stat wrapped away in a function and distinguish flavors there? That's in coreutils, after all.

export SDKMAN_DIR="$HOME/.sdkman"
fi

Expand Down