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

Fix mycroft-gui-core loader and stop #67

Merged
merged 1 commit into from
Sep 6, 2020
Merged

Fix mycroft-gui-core loader and stop #67

merged 1 commit into from
Sep 6, 2020

Conversation

PureTryOut
Copy link
Contributor

The conditional statement in the stop script was never actually closed

Also fix some shellcheck issues while we're at it

The conditional statement in the stop script was never actually closed

Also fix some shellcheck issues while we're at it
@forslund
Copy link
Contributor

forslund commented Sep 6, 2020

Nice catch! Should the exit be exit 1 indicating an error?

@PureTryOut
Copy link
Contributor Author

Yes, shellcheck mentions it and it's to make sure we exit in case cd'ing fails. E.g. the directory might for whatever reason not exist anymore and otherwise the ./stop-mycroft.sh call would fail. Catching the cd itself resolves that.

@forslund
Copy link
Contributor

forslund commented Sep 6, 2020

Yeah I understand the reason, just thinking that instead of a bare exit (which returns 0 if I recall correctly) it should call with a non-zero code to indicate exit on failure

@PureTryOut
Copy link
Contributor Author

Ah, yeah I guess so. Will change

@forslund
Copy link
Contributor

forslund commented Sep 6, 2020

Ah it uses the last exit code. I take it all back :)

@forslund
Copy link
Contributor

forslund commented Sep 6, 2020

It's fine as is. I was wrong

@forslund forslund merged commit 4b2d5f7 into MycroftAI:master Sep 6, 2020
@PureTryOut PureTryOut deleted the mycroft-gui-core-loader branch September 6, 2020 13:53
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.

2 participants