Fix scripts #3667

redianthus · 2017-06-26T17:55:51Z

Hi,

PhysSong · 2017-06-26T22:10:28Z

Great! Travis CI build is now passing.

tresf · 2017-06-27T00:28:02Z

cmake/msys/msys_helper.sh

-	mv "$mingw_root/lib/bin/libgig-7.dll" "$mingw_root/bin"

-	if [ $? -ne 0 ]; then
+	if ! mv "$mingw_root/lib/bin/libgig-7.dll" "$mingw_root/bin"; then


This is where shellcheck shows its value. If we need both DLLs, we'll need to test for both.

I agree. This problem also existed before, and we need to fix it if needed.

if ! ( mv "$mingw_root/lib/bin/libakai-0.dll" "$mingw_root/bin" && mv "$mingw_root/lib/bin/libgig-7.dll" "$mingw_root/bin" );then

@tresf Would it be better?

@PhysSong I'm not sure. I have some reservations about how the script is growing in general (could benefit from some utility functions). I guess it's OK. I'm curious what @zapashcanon comes up with.

There's something I don't understand. Why don't we (I) add set -e on the previous PR on this script ? That would allow us to remove almost all these tests...

@zapashcanon I think so.
If you add set -e, however, printing custom error message will be unable. Is there alternative method? Or, is it okay to disable printing custom error message?

Sure... we're moving toset -e for everything else. 👍. We've sort of repurposed a fetch/untar script as a build helper so this file is growing in complexity which can affect how it should respond to errors.

Ideally we'd just move away from this technique and leverage something crowdsourced like scoop instead, but we're not there yet.

I moved to set -e in a new commit. There's no need for custom error message IMO: we print a lot of info about what we're doing, so the user will know what failed, plus, the shell will print something too. What do you think ?

PhysSong · 2017-07-02T14:02:24Z

@tresf I suggest you to merge ASAP if there's no problem. Some people might think Travis CI is failing because of Critical bug(s). By merging this, We can prevent such misunderstandings.

tresf · 2017-07-02T14:33:08Z

The message to relaunch in mingw shell is there for a reasons since this can be unobvious to the user. Please bring it back as well as red error message.

redianthus · 2017-07-02T17:16:20Z

as well as red error message

Are you just talking about the relauch in mingw shell or every error message ?

I think most of the error message are useless, e.g.:

info "Downloading and building fltk $fltkver"

 if ! which fluid; then
    wget http://fltk.org/pub/fltk/$fltkver/fltk-$fltkver-source.tar.gz -O "$HOME/fltk-source.tar.gz"

If wget fails, no need to do: err "ERROR: Could not download fltk. Exiting." IMO, we already told the user what we're doing, if it fails, it will be clear.

If you disagree, so, what you want is just my first commit right ? I can just remove the second commit... But I think we should keep it.

tresf · 2017-07-02T17:18:56Z

I think most of the error message are useless [...] If wget fails, no need to do: err ...

Agreed, just the mingw one, it's not obvious at all, but to that point we may have others that aren't obvious down the road. set -e is a convenience, but we have to keep in the ability to issue a human warning (or error) when stderr is inadequate. From my experience, stderr failing to tell the ACTUAL problem happens about 10% of the time.

redianthus · 2017-07-02T17:43:57Z

we have to keep in the ability to issue a human warning (or error) when stderr is inadequate

We probably could do something with trap.

Agreed, just the mingw one

I'll fix it in the second commit then.

PhysSong · 2017-07-04T04:57:10Z

@zapashcanon @tresf Now it seems to be ready for merge. Is it needs any more changes?

tresf · 2017-07-04T05:05:29Z

Were the indents addressed? I understand there is a need for master to clear Travis-CI, but "ready for merge" is subjective.

PhysSong · 2017-07-04T05:15:01Z

cmake/msys/msys_helper.sh

-	if [ $? -ne 0 ]; then
-        	err "ERROR: Could not build/install libgig -- gigplayer needs this.  Exiting."
-	fi
+  mv "$mingw_root/lib/bin/libgig-7.dll" "$mingw_root/bin"


Needs indentation?

PhysSong · 2017-07-04T05:16:56Z

.travis/script.sh


+  # SC2185 is disabled because of: https://github.com/koalaman/shellcheck/issues/942
+  # once it's fixed, it should be enabled again
+  # shellcheck disable=SC2185


Here three lines, too.

redianthus · 2017-07-04T09:18:07Z

@tresf, I didn't noticed the indent, fixed now.

tresf · 2017-07-04T14:18:21Z

Thanks for the fixes! Merging. If we encounter new issues we'll submit them as new PRs/patches.

tresf · 2017-07-04T14:21:27Z

@zapashcanon the merge button gave credit to me do to the way the commits were added (my attempt to Jeep then separate). Apologies.

zonkmachine · 2017-07-04T14:34:25Z

the merge button gave credit to me do to the way the commits were added

git commit --amend --author="Author Name <[email protected]>"

zonkmachine · 2017-07-04T14:40:33Z

@tresf No. He's credited. This was in master and our default is stable-1.2 so once we switch to master these commits will be visible. If I'm even on topic here...

tresf · 2017-07-04T14:48:28Z

@zonkmachine you're right. 👍

https://github.com/LMMS/lmms/commits/master

redianthus · 2017-07-04T15:04:19Z

Merging.

Thanks.

If we encounter new issues we'll submit them as new PRs/patches.

I agree on this.

Fix scripts for shellcheck

tresf reviewed Jun 27, 2017

View reviewed changes

PhysSong reviewed Jul 4, 2017

View reviewed changes

Fix scripts

e90bc63

Clean code

060c012

tresf merged commit 94420d9 into LMMS:master Jul 4, 2017

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Merge pull request LMMS#3667 from zapashcanon/scripts

a6bb282

Fix scripts for shellcheck

Uh oh!

Fix scripts #3667

Fix scripts #3667

Uh oh!

Conversation

redianthus commented Jun 26, 2017

Uh oh!

PhysSong commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tresf Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhysSong commented Jul 2, 2017

Uh oh!

tresf commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redianthus commented Jul 2, 2017

Uh oh!

tresf commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redianthus commented Jul 2, 2017

Uh oh!

PhysSong commented Jul 4, 2017

Uh oh!

tresf commented Jul 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redianthus commented Jul 4, 2017

Uh oh!

tresf commented Jul 4, 2017

Uh oh!

tresf commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zonkmachine commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zonkmachine commented Jul 4, 2017

Uh oh!

tresf commented Jul 4, 2017

Uh oh!

redianthus commented Jul 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PhysSong commented Jun 26, 2017 •

edited

Loading

tresf Jun 27, 2017 •

edited

Loading

tresf commented Jul 2, 2017 •

edited

Loading

tresf commented Jul 2, 2017 •

edited

Loading

tresf commented Jul 4, 2017 •

edited

Loading

zonkmachine commented Jul 4, 2017 •

edited

Loading