Skip to content
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.

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

sed error in install script #773

Closed
ethankhall opened this issue Feb 4, 2024 · 2 comments · Fixed by #776
Closed

sed error in install script #773

ethankhall opened this issue Feb 4, 2024 · 2 comments · Fixed by #776

Comments

@ethankhall
Copy link

I'm using cargo-dist in a project, and was testing out the install script and saw an error message.

curl --proto '=https' --tlsv1.2 -LsSf https://github.com/ethankhall/scope/releases/download/v2024.2.4/dev-scope-installer.sh | sh
sed: 1: "s,"CARGO_DIST_BINS","sc ...": bad flag in substitute command: '"'

The install looks to have worked, but I would expect to not see this error message.

System Details

❯ echo $SHELL
/bin/zsh
❯ zsh --version
zsh 5.9 (x86_64-apple-darwin23.0)
❯ sh --version
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin23)
Copyright (C) 2007 Free Software Foundation, Inc.
@Gankra
Copy link
Contributor

Gankra commented Feb 6, 2024

Ah shoot

    RECEIPT="$(echo "$RECEIPT" | sed s,'"CARGO_DIST_BINS"',"$_bins_js_array",)"

This sed uses , instead of / to avoid the obvious problems of path separators, but then runs into the fact that we represent a list of binaries as a,b... which makes using , as a delimiter freak out.

This only affects "install receipts" which is a ground-work feature for future updater work. So your installer works fine but gets that ugly message. Definitely need to fix this and cut a quick release -- thanks for reporting!!

@Gankra
Copy link
Contributor

Gankra commented Feb 6, 2024

good news, we have tests that cover this situation

_bins_js_array='"akextract","akmetadata","akrepack"'

bad news, they don't check that the install receipt got installed (time to add that)

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 a pull request may close this issue.

2 participants