adds switch for macos systems to use greadlink instead#1885
adds switch for macos systems to use greadlink instead#1885mommel wants to merge 4 commits intodocker-mailserver:masterfrom mommel:fixes-readlink-issue-for-darwin
Conversation
casperklein
added
area/enhancement
kind/improvement
|
Thank you for the contribution 👍 However there were two bugs:
I fixed both issues. Please review and test my changes on MacOS. |
|
This PR fixes a problem which should in theory not even exist. Looking at the code, the failure of calling |
| if uname -a | grep -q Darwin | ||
| then | ||
| CDIR="$(dirname "$(readlink -f "${0}")")" | ||
| SCRIPT_PATH="$(greadlink -f "${0}")" |
There was a problem hiding this comment.
We should not use greadlink. We do not officially support macOS. The script should work in a way which
- Checks if
dirname "$(readlink -f "${0}")"works, if not, go on - Checks if
realpath -e -L "${0}"works, if not, use the value provided on initialization, which is - The value of
$(pwd)
There was a problem hiding this comment.
I agree. That was one of the reasons, I haven't approved this PR yet. I don't have a Mac to test with. That would imply, that neither pwd nor realpath is available in @mommel's environment, which sounds strange.
There was a problem hiding this comment.
pwd does exist on Big Sur, realpath does not.
There was a problem hiding this comment.
@fbartels Thanks for pointing this out! But as long as one of the three commands work, this should not be a problem in the end, i.e. I still have no clue as to why this should not work :)
There was a problem hiding this comment.
What about getting rid of the whole _get_current_directory function? 😄
CDIR=${0%/*} should all we need. Pure Bash and no dependencies to other binarys.
There was a problem hiding this comment.
What about getting rid of the whole
_get_current_directoryfunction? smile
CDIR=${0%/*}should all we need. Pure Bash and no dependencies to other binarys.
Quite nice, but only under the condition that working with relative paths is okay. I do not see an problems at the moment, but I'm not 100% confident either.
There was a problem hiding this comment.
Yes, I've considered this. In the current code, there is no need for an absolute path.
There was a problem hiding this comment.
Yes, I've considered this. In the current code, there is no need for an absolute path.
Although we're "losing" the capability to resolve softlinks, this should not be an issue altogether. I will close this PR. Can you provide a new one which implements this behavior?
There was a problem hiding this comment.
Unfortunately, an absolute path is required for docker -v command 😕
|
This would need rebasing. But I guess opening a new PR (if still needed) is easier. |
adds switch for macos systems to use greadlink instead Issue on macos is readlink does not support -f greadlink instead does
4 participants
Issue on macos is readlink does not support -f greadlink instead does
Description
readlink on macos does not support -f
Fixes # (issue)
#1884
Type of change
Checklist: