search

alcatraz / Alcatraz

Package manager for Xcode
alcatraz.io
MIT License
9.87k stars 1.15k forks source link

Fix install script when no plugin was ever installed in Xcode #393

Closed guillaumealgis closed 8 years ago

guillaumealgis commented 8 years ago

Fix https://github.com/alcatraz/Alcatraz/issues/392 #394

If Xcode never loaded any plugin, the DVTPlugInManagerNonApplePlugIns-Xcode-$xcode_version does not exist, and the script halted on the line 

defaults read -app Xcode "$PLIST_PLUGINS_KEY" > "$TMP_FILE"

We now check the return code of default read before continuing.

Sorry about that :/

jurre commented 8 years ago

:+1: thanks @guillaume-algis

@supermarin @kattrali LGTM

supermarin commented 8 years ago

Thanks @guillaume-algis!

Looks good, altho I'd prefer using set -e for control flow. Merging to fix existing installations

guillaumealgis commented 8 years ago

Looks good, altho I'd prefer using set -e for control flow.

Not quite sure what you mean. We're already using set -e. This is precisely why the script failed for some people. defaults read returned 1 on the non-existing key, and set -e halted the script.

supermarin commented 8 years ago

Oh sorry, I reviewed from the phone. Not sure how the && fixes the problem then https://github.com/alcatraz/Alcatraz/pull/393/files?diff=split#diff-19783275db54420a49d7297671f8258eR13

Shouldn't the line fail on set -e the same way && wouldn't continue?

guillaumealgis commented 8 years ago

You got me worried here for a minute. No, && prevent set -e to halt the script (emph. mine):

The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or || [...]

So by using && we explicitly allow defaults read to fail (which is reasonable). I just realized the script is still printing the defaults read error though. A 2> /dev/null is probably needed.