Use the current user's SDKMAN installation #718
Conversation
When `su`-ing to another user, environment variables carry over. As a result, `sdk` will point to the installation of the original user, not the one running the current shell. Thus: Reset `$SDKMAN_DIR` if the directory it points at belongs to a user other than the current one.
reitzig
changed the title
src/main/bash/sdkman-init.sh
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, and of course, please refrain from using
awkas 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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Actually, yes I do think so. We use double square brackets throughout the sdkman codebase when dealing with complex expressions.
There was a problem hiding this comment.
The
cutcommand 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.
|
It occurs to me that |
|
To run a command as another user, don't use |
When
su-ing to another user, environment variables carry over.As a result,
sdkwill point to the installation of the original user,not the one running the current shell.
Thus: Reset
$SDKMAN_DIRif the directory it points atbelongs to a user other than the current one.
(Sorry, I didn't have the time to fulfill the prerequisites. I empathize, but that doesn't change my time prioritization.)