Skip to content

fix: fix install script informational echo's - #1162

Merged
kvinwang merged 4 commits into
Dstack-TEE:nextfrom
WimYedema:fix/install-script-resolve-source
Sep 4, 2026
Merged

fix: fix install script informational echo's#1162
kvinwang merged 4 commits into
Dstack-TEE:nextfrom
WimYedema:fix/install-script-resolve-source

Conversation

@WimYedema

Copy link
Copy Markdown
Contributor

The resolve_source function echo's a return value but also informational messages. I created an info function for the informational messages such that the return value is not muddied. For completeness-sake I also added an error function.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new info()/error() helpers use echo, which is not fully portable in POSIX /bin/sh for option-like leading arguments (e.g., --prefix), so switching to printf would avoid inconsistent behavior across shells.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors dstack/scripts/install.sh output so resolve_source() can safely return a checkout path on stdout without being interleaved with informational/status messages.

Changes:

  • Introduces info() (stderr) and error() (stderr with error: prefix) helper functions.
  • Replaces direct echo "error: ..." sites with error ..., and routes resolve_source() progress messages through info ....
File summaries
File Description
dstack/scripts/install.sh Adds info/error helpers and uses them to keep resolve_source() stdout clean for its return value.
Review details

Suppressed comments (1)

dstack/scripts/install.sh:66

  • Same portability concern as info(): echo has implementation-defined behavior for leading - arguments. Use printf here too so option-like messages (e.g., --repo ...) are never misinterpreted.
error() {
    echo "error: $*" >&2
}
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dstack/scripts/install.sh
@kvinwang

kvinwang commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Pushed a follow-up commit to this branch (9e1b99d) — hope you don't mind, maintainerCanModify was on.

The stdout leak isn't only from the script's own echos

resolve_source() returns the checkout path on stdout, so anything written there lands in $checkout. git writes to stdout too:

  • git checkout <branch> prints Switched to branch/Already on to stderr, but the branch-tracking line Your branch is up to date with 'origin/next'. to stdout.
  • git pull --ff-only prints its Updating ../Fast-forward summary to stdout.

So the branch as submitted still broke, including on the default curl | sh path (--ref defaults to the next branch; a tag/SHA happens to be fine because detached-HEAD messages all go to stderr):

$ sh install.sh --src /tmp/e2esrc --repo ... --ref next --prefix ... --no-sudo
cloning dstack source into /tmp/e2esrc
Cloning into '/tmp/e2esrc'...
Already on 'next'
building dstackup from Your branch is up to date with 'origin/next'.
/tmp/e2esrc
install.sh: 268: cd: can't cd to Your branch is up to date with 'origin/next'.
/tmp/e2esrc
exit=2

The fix redirects the git subshells and the git clones as a whole () >&2) rather than adding -q per command, so a git call added later can't reintroduce the same bug.

Copilot's printf note

Applied — it's a real difference, not just style. Under dash, info "-n" with echo swallows the line entirely:

$ dash -c 'info() { echo "$*" >&2; }; info "-n"; info "-e whatever"'
-e whatever          # "-n" vanished, "-e" consumed as a flag
$ dash -c 'info() { printf "%s\n" "$*" >&2; }; info "-n"; info "-e whatever"'
-n
-e whatever

Also routed building dstackup from ... through info for consistency; the final install summary stays on stdout.

Verification

Harness that truncates the script after resolve_source() and checks that stdout is exactly the path, across all six resolution paths:

Test base (pre-PR) this PR before after
temp clone (curl | sh) FAIL FAIL PASS
--src fresh clone FAIL FAIL PASS
--src update (git pull) FAIL FAIL PASS
--src --ref <tag> FAIL PASS PASS
cwd is a checkout PASS PASS PASS
script_checkout via $0 PASS PASS PASS

Plus a full end-to-end run with a stubbed cargo (clone → build → install_bin → summary): exits 0, binary installed, stdout carries only the final summary, all progress on stderr. prek run --from-ref <base> --to-ref HEAD passes, shellcheck included.

resolve_source() returns the checkout path on stdout, so anything else
written there ends up in $checkout. Routing the script's own progress
messages to stderr is not enough: git writes to stdout too. 'git checkout
<branch>' prints the branch-tracking line ('Your branch is up to date
with ...') and 'git pull' prints its fast-forward summary, both on
stdout. That leaves $checkout as a multi-line string, so core_dir() no
longer matches and the following 'cd "$core_checkout"' fails - including
on the default 'curl | sh' path, where --ref defaults to the 'next'
branch.

Redirect the git subshells and clones as a whole rather than adding -q
per command, so a git call added later cannot reintroduce the same bug.

Also use printf instead of echo in info()/error(): echo's handling of a
leading '-' is implementation-defined, and dash swallows 'info "-n"'
entirely.
@kvinwang
kvinwang force-pushed the fix/install-script-resolve-source branch from 9e1b99d to fb5310a Compare September 4, 2026 10:08
@kvinwang
kvinwang enabled auto-merge September 4, 2026 10:15
@kvinwang
kvinwang merged commit e7703dd into Dstack-TEE:next Sep 4, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants