fix: relative symlink resolution in install script - #1244
fix: relative symlink resolution in install script#1244V-Subhankar-infy wants to merge 5 commits into
Conversation
|
Beware that --- a/devcontainer 2026-06-19 15:12:58
+++ b/devcontainer 2026-06-19 15:13:11
@@ -6,15 +6,22 @@
# Resolve the installation directory
# Handle both direct execution and symlinked scenarios
-if [ -L "$0" ]; then
- # Follow symlink
- SCRIPT_PATH="$(readlink "$0" 2>/dev/null || readlink -f "$0" 2>/dev/null || echo "$0")"
-else
- SCRIPT_PATH="$0"
-fi
-
-# Get absolute path to script directory
-SCRIPT_DIR="$(cd "$(dirname "$SCRIPT_PATH")" && pwd)"
+SCRIPT_DIR="$(
+ CDPATH=
+ self=$0
+
+ while [ -L "$self" ]; do
+ cd -- "${self%/*}" >/dev/null || :
+ self=$(readlink "${self##*/}")
+ done
+
+ case "$self" in
+ (*/*) ;;
+ (*) self=./$self ;;
+ esac
+
+ cd -P -- "${self%/*}" >/dev/null && pwd
+)"
INSTALL_DIR="$(dirname "$SCRIPT_DIR")"
# Paths to bundled components(Reference/explanation: https://stackoverflow.com/a/246128/1968) |
d34e588 to
e612da1
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the generated devcontainer wrapper’s self-location logic so that relative symlink targets are resolved relative to the symlink’s directory (not the caller’s CWD), and adds a regression test covering the reported split-local-layout scenario.
Changes:
- Replace the wrapper’s single-hop
readlink/readlink -fresolution with a hop-by-hop symlink-chain resolver. - Canonicalize the resolved script directory before deriving
INSTALL_DIR. - Add an integration regression test for invoking a relative symlinked wrapper from a different working directory.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.sh | Reworks wrapper script-location resolution to correctly handle relative symlink chains without readlink -f. |
| scripts/install.test.sh | Adds a regression test for relative symlink invocation from a different directory. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
scripts/install.sh:444
cd -P --anddirname --are not portable;--is not specified by POSIX forcdordirnameand can be interpreted as a literal path/operand on some systems. SinceSCRIPT_DIRis already an absolute path, dropping--is safe and avoids breaking the wrapper on macOS/BSD and dash-based /bin/sh.
cd -P -- "${self%/*}" >/dev/null && pwd
)"
INSTALL_DIR="$(dirname -- "$SCRIPT_DIR")"
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
scripts/install.sh:443
cdin POSIX sh does not specify support for a--end-of-options marker. Usingcd -P -- ...can be interpreted as trying tocdinto a directory literally named--on some shells, which would makeSCRIPT_DIRresolution fail (and the wrapper exit due toset -e).
cd -P -- "${self%/*}" >/dev/null && pwd
scripts/install.sh:445
dirnameis specified without options in POSIX, sodirname -- "$SCRIPT_DIR"is not portable: on systems wheredirnametreats--as a path operand (common on BSD/macOS), this can emit an extra line and corruptINSTALL_DIR. SinceSCRIPT_DIRcomes frompwd, it won't start with-anyway, so--is unnecessary.
INSTALL_DIR="$(dirname -- "$SCRIPT_DIR")"
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
| # Get absolute path to script directory | ||
| SCRIPT_DIR="$(cd "$(dirname "$SCRIPT_PATH")" && pwd)" | ||
| INSTALL_DIR="$(dirname "$SCRIPT_DIR")" | ||
| cd -P -- "${self%/*}" >/dev/null && pwd |
There was a problem hiding this comment.
I don't think cd -- would work on POSIX/macOS/BSD environments.
There was a problem hiding this comment.
The cd -- (option terminator) has been part of systems since 1995 and I have just checked on zsh shell in my macos and it supports this.
To be precise, this is completely safe to use cd -- on any system from 2005 onward:
- macOS 10.0+ (2001+)
- FreeBSD 6.0+ (2005+)
- OpenBSD 5.0+ (2008+)
- NetBSD 1.5+ (1999+)
I am not aware of how much backward compatibility do we need to have, but it seems like a valid decision, considering readlink even the basic version is relatively more recent.
Summary
Fixes relative symlink resolution in the install script for split local layouts (e.g. a launcher in
~/.local/binpointing to a payload in~/.local/lib/devcontainers). The wrapper now resolves its own location with portable POSIX shell built-ins instead of GNU-specificreadlink -f, sodevcontainerworks regardless of the caller's working directory and shell environment (Linux, macOS, BSD).Fixes #1232
Root Cause
A single
readlinkcall pluscd "$(dirname ...)" && pwdresolved relative symlink targets against the caller's CWD rather than the link's own directory, and thereadlink -ffallback isn't portable to macOS/BSD.Reproduction of error
Fix
The script-location resolver now walks the symlink chain one hop at a time,
cd-ing into each link's directory before reading the next target, then canonicalizes the final directory. This replaces the previous singlereadlinkresolution.Why It Works
cd -P … && pwdfor an absolute, canonical path.readlink,${var%/*},cd -P,pwd) — noreadlink -f/realpath.CDPATH=cleared; no leakedcdor stray output.Validation
Regression test confirms a relative symlink launched from another directory exits
0and reports the installed CLI version.Scope & Risk
One resolution block; no install/download changes. Direct execution and absolute symlinks are unchanged. Low risk, plus improved cross-platform portability.
Credits
Thanks to @klmr for the portable POSIX symlink-resolution approach.