Add shellcheck to the build #3476

zapashcanon · 2017-04-01T22:18:22Z

Hi,

Just add a build called 'style', which will be used for kwstyle and astyle too (I hope at least).

Currently it's checking for each .sh file in .travis/ and in .cmake/, others scripts are not ours so we won't check against them. If there are others scripts I forgot, just tell me.

I also moved some of the code in .travis.yml to separated .sh files as @jasp00 advised, so they can be checked (shellcheck found a few things in them btw) and also to ease the process of adding a "style build" which has nothing in common with other builds.

tresf · 2017-04-01T22:51:54Z

.travis.yml

@@ -49,4 +46,4 @@ notifications:
           - https://webhooks.gitter.im/e/1ac7fc698195981a9227


Unrelated to this PR, but we don't use gitter anymore. We can probably shut this down.

I removed the whole section as it looked like it was only for gitter.

tresf · 2017-04-01T22:55:53Z

.travis/before_script.sh

+
+  export CMAKE_FLAGS="-DWANT_QT5=$QT5 -DUSE_WERROR=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo"
+
+  if [ -z "$TRAVIS_TAG" ]; then export CMAKE_FLAGS="$CMAKE_FLAGS -DUSE_CCACHE=ON"; fi


This was likely kept a one-liner to keep the .yml clean reading. Please break this out now that it's in a shell script. :)

tresf · 2017-04-01T22:56:32Z

.travis/install.sh

+#!/usr/bin/env bash
+
+if [ "$TYPE" = 'style' ]; then
+  sudo apt install shellcheck


Should this be apt-get ?

From the debian admin. handbook:

APT is a vast project, whose original plans included a graphical interface. It is based on a library which contains the core application, and apt-get is the first front end — command-line based — which was developed within the project. apt is a second command-line based front end provided by APT which overcomes some design mistakes of apt-get.

IMHO using apt is better.

Thanks. We use apt-get everywhere else, let's use that for consistency. If there turns out to be advantages using apt over apt-get, we'll switch the whole project to use it in a separate task (wiki, travis, etc).

tresf · 2017-04-01T23:00:41Z

Would it make more sense to force this as a Linux-environment run? In the current state, it's assuming items like apt are available, which really shouldn't happen on anything except Linux environments.

zapashcanon · 2017-04-02T00:25:02Z

to force this as a Linux-environment

It's taking the default from .travis.yml which is dist: trusty so I don't think it's necessary.

tresf · 2017-04-02T01:01:28Z

It's taking the default from .travis.yml which is dist: trusty so I don't think it's necessary.

Fair enough, but these should still only live in the linux scripts and you can just add another block to the apt-get logic rather than segmenting them which also addresses https://github.com/LMMS/lmms/pull/3476/files#r109296139.

tresf · 2017-04-02T01:05:07Z

live in the linux scripts

Moving them to the Linux scripts may also benefit us if some of these tools shift from Linux to homebrew, etc as history has proven sometimes Homebrew has newer versions of packages which would change apt-get to brew install directly in the respective install files.

zapashcanon · 2017-04-02T01:15:00Z

these should still only live in the linux scripts and you can just add another block to the apt-get logic rather than segmenting them

So, you wan't me only to merge the apt-get part ? I'm not sure because, for others files, like .travis/script.sh they're not merged, would make it harder to understand. So I'd rather merge all of them, which will be more complicated in the case of .travis/script.sh, or even better, not merge them at all and stay like this which seems clearer to me.

which also addresses https://github.com/LMMS/lmms/pull/3476/files#r109296139.

When I click here, I only get a diff. Not sure what you're talking about here.

Adding them to Linus may also benefit us if some of these tools shift from Linux to homebrew, etc as history has proven sometimes non-Ubuntu packages sometimes run newer.

I don't understand sorry. Why adding them to linux would help if one day there's a more up to date homebrew version ?

tresf · 2017-04-02T01:28:21Z

would make it harder to understand

I thought of that too. I think it's fine. That way all CI platforms can have the formatting if needed. It's more granular.

for others files, like .travis/script.sh

The .yml really can go back. I don't see a reason they can't live directly in the platforms scripts that they run on.

I don't understand sorry.

I realized that and had edited my post. :) The point was if a utility that we're using for style or code quality is dated, often the Homebrew environment will get the bleeding edge first. This has happened time and time again (Qt5, GigPlayer, etc) so having apt-get calls in the root of install when they aren't platform independent seems like it could have scalability problem if we switch. Just a forethought.

When I click here, I only get a diff.

It got squashed out. Was the apt-get comment, which has since been fixed anyway. 👍

zapashcanon · 2017-04-02T01:41:22Z

I thought of that too. I think it's fine. That way all CI platforms can have the formatting if needed. It's more granular.

Sorry, I don't understand what you mean. But, what would you all the CI platforms to have the formatting ? And I don't understand what you want me to do.

The .yml really can go back. I don't see a reason they can't live directly in the platforms scripts that they run on.

Because now, shellcheck is going to check those files. In .travis/before_script.sh as an example it spotted: mkdir build && cd build and told me to replace it by: mkdir build && cd build || exit (in case cd fails).

My point is really that we have build builds for many platform and a style build which should be separated. If someday we decide we don't want to run this style build on trusty, we will just have to do something like:

        - os: osx
          env: TYPE=style

And just edit the install.sh file. Seems more clear to me than mixing build builds and style build.

tresf · 2017-04-02T02:05:30Z

I think the || exit could be improved. We should return non-zero, probably set -e

The other points make sense and you're right they should stay for now. Thanks!

jasp00

Some improvements.

jasp00 · 2017-04-02T08:44:34Z

.travis.yml

@@ -18,17 +19,13 @@ matrix:
        - os: osx
          env: QT5=True
 before_install:
-   - . ${TRAVIS_BUILD_DIR}/.travis/${TRAVIS_OS_NAME}.${TARGET_OS}.before_install.sh
+    - . ${TRAVIS_BUILD_DIR}/.travis/before_install.sh


You should try to remove all this sourcing and use single lines.

I'm not sure what you mean by use single lines.

install: ${TRAVIS_BUILD_DIR}/.travis/install.sh

Doesn't work, have to do a chmod or call bash directly. So, here, I choosed to do: bash file.

It will work if you set execute permissions.

Yep', so if you want it to fit in a single line I'll have to do something like: chmod +x files && file, here, using bash file looked better to me.

Do you know how to set permissions in the repository or not?

Yep, just did it. I didn't know it was possible to do this with git.

jasp00 · 2017-04-02T08:52:25Z

.travis/before_install.sh

+
+if [ "$TYPE" != 'style' ]; then
+  # shellcheck disable=SC1090
+  . "$TRAVIS_BUILD_DIR/.travis/$TRAVIS_OS_NAME.$TARGET_OS.before_install.sh"


Would you mind indenting with tabs, such as in .travis/linux..install.sh?
You should try to remove sourcing.

Done for the indenting part.

How do you want me to remove sourcing ? I could do:

chmod +x file && ./file or bash file, no idea what's best.

You should set permissions in the repository, like cmake/build_mingw32.sh.

I'm not sure what you meant, so I just did a chmod.

jasp00 · 2017-04-02T08:57:08Z

.travis/before_script.sh

+  if [ -z "$TRAVIS_TAG" ]; then
+    export CMAKE_FLAGS="$CMAKE_FLAGS -DUSE_CCACHE=ON"
+  fi
+fi


You should merge this script with .travis/script.sh.

jasp00 · 2017-04-02T09:10:02Z

.travis.yml

-    - . ${TRAVIS_BUILD_DIR}/.travis/${TRAVIS_OS_NAME}.${TARGET_OS}.script.sh
-    - make -j4
-    - if [[ $TARGET_OS != win* ]]; then make tests && ./tests/tests; fi;
+    - . ${TRAVIS_BUILD_DIR}/.travis/script.sh
 after_script:
    - ccache -s


You should move this command to .travis/script.sh.

No you shouldn't. Moving it to script will cause it to affect the build result.

Moving it to script will cause it to affect the build result.

I am assuming @zapashcanon will drop sourcing and act accordingly. ccache -s prints a statistics summary, are you worried that ccache may fail?

I don't see how sourcing relates to this.

are you worried that ccache may fail?

If it fails, I don't want our CI to fail because of that, obviously.

Regardless, it doesn't belong in script because it's not part of our tests.

@zapashcanon, you should move this command to .travis/after_script.sh and execute ccache when appropriate.

By appropriate, you mean not when running the style build right ?

I guess so. If ShellCheck does not compile, then ccache makes no sense.

Seems a little bit useless to me. ccache indeed makes no sense, but it'll just print: "nothing changed" and it's done. Is it worth to add a ~10 lines script when keeping it in one line is the same ?

If ccache -s output is irrelevant, then you should not run ccache -s. By ~10 lines, do you mean 2? No matter how many lines, it is worth because ShellCheck will check the script.

jasp00 · 2017-04-02T09:17:46Z

.travis/before_install.sh

+if [ "$TYPE" != 'style' ]; then
+  # shellcheck disable=SC1090
+  . "$TRAVIS_BUILD_DIR/.travis/$TRAVIS_OS_NAME.$TARGET_OS.before_install.sh"
+fi


You may merge this script with .travis/install.sh.

jasp00 · 2017-04-02T09:20:32Z

.travis/script.sh

+  if [[ $TARGET_OS != win* ]]; then
+
+    make tests
+    ./tests/tests


Why the dot?

No idea, that's how it was before in travis.yml.

I see. Plain tests/tests should work.

zapashcanon · 2017-04-02T15:05:07Z

@jasp00, some of your comments are now on outdated code but not fixed. Just to make sure you don't miss my comments.

zapashcanon · 2017-04-08T00:50:06Z

@tresf @jasp00, I think I'm done with each review. Anything missing ?

tresf · 2017-04-08T02:41:28Z

@zapashcanon the chmod +x technique is fine. Have we also considered trying fixing this flag on the file itself?

zapashcanon · 2017-04-09T16:16:04Z

@tresf setting this flag seems interesting. Do you want me to do it ? Otherwise, I'd like this to get merged so I can start adding things to the style job.

jasp00 · 2017-04-09T21:12:00Z

@tresf setting this flag seems interesting. Do you want me to do it ?

Please, set the flag and drop chmod calls.

tresf · 2017-04-10T13:11:16Z

@zapashcanon thanks. I know some of these things seem mundane, but it makes the feature cleaner as a whole and sets a good example (such as the execute flag). Thanks for the patience with this.

tresf · 2017-04-10T13:12:23Z

.travis/install.sh

+	sudo apt-get -yqq update
+	sudo apt-get install shellcheck
+else
+


Minor, but there seem to be some additional newlines in this else block as well as the the if [ ] block in the after_script.sh file.

zapashcanon · 2017-04-10T13:17:54Z

@tresf, it's ok, I understand. I'm even glad to know now that git is able to handle permissions.

jasp00

Further improvements.

jasp00 · 2017-04-10T13:58:09Z

.travis/install.sh

@@ -0,0 +1,9 @@
+#!/usr/bin/env bash
+


You should add set -e to all scripts except after_script.sh.

I'm not sure why I shouldn't add it into after_script.sh. If after_script.sh returns a non-0 exit code, Travis won't say the build failed. That's exactly the purpose of the after_script in the .yml file.

It won't hurt in after_script but to @jasp00's point, the ccache statistics aren't really critical for a pass/fail and it's a single-line, so set -e on the whole script hasn't yet been warranted over something like exit $?. But @zapashcanon you're right, it's best to add it.

As I see after_script, it is useful for statistics, non-critical tests, custom notifications (not webhooks), dumps, etc. You probably want to run all of them, so set -e is not convenient. Right now, it does not matter.

Could you add set -e to linux..before_install.sh, linux.win32.before_install.sh, linux.win32.install.sh, etc.?

Just did it.

jasp00 · 2017-04-10T14:01:13Z

.travis/script.sh

+
+else
+
+	mkdir build && cd build || exit


Could you use simple commands and drop exit?

mkdir build && cd build was here before in the .yml. I added the exit but with set -e it's useless now.

tresf · 2017-04-11T05:01:36Z

@zapashcanon Homebrew seems to be picky about the install command. If a package is already installed, it returns a non-zero code and bombs out the script. This is now exposed by set -e.

I'd recommend we keep the package listing incase we need to invoke this from another build system and make it variable.

# skip preinstalled packages
if ! "${TRAVIS}"; then
   PACKAGES="$PACKAGES cmake pkgconfig node"
fi

Alternately, I believe brew reinstall will return warning instead of error, but it also adds unecessary time reinstalling packages that are already installed.

zapashcanon · 2017-04-11T18:29:03Z

@tresf, I tried something else, which seems better to me. Currrently not working, but I'll fix it tomorrow. Do you agree with the idea ? I'm getting the list of already installed packages from $PACKAGES, then I remove them from $PACKAGES and finally install.

Moreover, there's this error too: Error: No such keg: /usr/local/Cellar/fftw. I've no idea what this is as I (almost) don't use MacOS or homebrew. Any clue ?

tresf · 2017-04-11T18:37:26Z

Do you agree with the idea

Absolutely. This is what I find recommended most places. It could even be extended to install them one-by-one and remove the bleeding $FOO, which would probably make @jasp00 happy.

No such keg: /usr/local/Cellar/fftw

It's simply the first already-installed package... Yet another side-effect of set -e I'm afraid. I think we need to do something like p in PACKAGES and catch errors from brew list (|| true or whatever). Probably don't even need to concatenate, just use the conditional to call brew install one by one. Man what a shitty side effect this !0 is causing. :)

zapashcanon · 2017-04-11T18:47:23Z

to install them one-by-one

I thought about that, but I was afraid it would make the installation process longer.

tresf · 2017-04-11T18:52:46Z

I thought about that, but I was afraid it would make the installation process longer.

It shouldn't.

tresf · 2017-04-11T20:12:22Z

I was wrong @zapashcanon... it does make a difference.

Here's my benchmarking

`foo bar`	`foo && bar`
9.357s	12.21s
7.596s	10.598s
8.261s	12.209

Probably best to build out the string in this case.

zapashcanon · 2017-04-14T15:17:31Z

@tresf, I replaced for p in $(brew list $PACKAGES); do by for p in $(brew list); do. It worked. I also had to replace pkgconfig by pkg-config.

Now it fails on: branch=$(git symbolic-ref --short HEAD), I tried something but it didn't work. I'm not sure how to fix this. As you're the one who wrote this, have you any idea ?

tresf · 2017-04-14T16:30:32Z

@zapashcanon the git command should be valid, it just gets the current branch name. Custom homebrew formulas are hosted .rb files, so we have to point to the GitHub URL and that part makes sure it's the stable-1.2, master, etc.

I'm guessing the script just doesn't know where the git repository is. Can you try something like push ${TRAVIS_BUILD_DIR} (or cd, etc) and see if that resolves it?

zapashcanon · 2017-04-14T17:03:34Z

I tried cd $TRAVIS_BUILD_DIR, didn't work.

I'm not sure the command is valid, see this. Looks like we're in a detached HEAD state, no ? I don't know how travis get the git repo. etc.

zapashcanon · 2017-04-14T17:20:33Z

git-symbolic-ref:

git symbolic-ref will exit with status 0 if the contents of the symbolic ref were printed correctly, with status 1 if the requested name is not a symbolic ref, or 128 if another error occurs.

Travis build log:

fatal: ref HEAD is not a symbolic ref

The command "${TRAVIS_BUILD_DIR}/.travis/install.sh" failed and exited with 128 during .

tresf · 2017-04-14T17:56:09Z

@zapashcanon it appears weren't not the only ones wanting this.

Can we add a fallback to use ${TRAVIS_PULL_REQUEST_BRANCH}? I say fallback because --
despite the home -- this script may some day be run outside of a travis environment. e.g. Our Mac tutorial currently recommends it although I'll soon be removing this in favor of a more comprehensive quickstart.

Back on topic... here's the Travis-CI documentation around the branch name:
travis-ci/travis-ci#1633 (comment)

zapashcanon · 2017-04-14T17:58:21Z

Ok, so, from the latest test:

pwd:

/Users/travis/build/LMMS/lmms

url=https://github.com/LMMS/lmms.git

git branch:

* (HEAD detached at FETCH_HEAD)

  master

git status:

HEAD detached at FETCH_HEAD

nothing to commit, working tree clean

fatal: ref HEAD is not a symbolic ref

So git symbolic-ref --short HEAD doesn't work because we're in detached head state, I've got no idea how to fix this properly.

I could do something like:

if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then
    branch="master"
else
    branch=$(git symbolic-ref --short HEAD)
fi

But it won't work for PR to stable-1.2 or stuff like this.

tresf · 2017-04-14T18:02:44Z

I think we're on the same page. It is paramount we know the branch name. If it becomes too much of a struggle we can research how Homebrew does local .rb files or hardlink a gist.

zapashcanon · 2017-04-14T20:42:10Z

@tresf, it works now, it's a bit hacky but it should do the trick.

tresf · 2017-04-14T20:50:54Z

That's perfect.

zapashcanon · 2017-04-14T21:38:26Z

So, do you see anything left to do before merging ?

tresf · 2017-04-15T01:22:20Z

@zapashcanon I found a script that we missed... cmake/apple/install_apple.sh.in (due to the non-matching *.sh). We can append -or -name '*.sh.in' to the find command to add it. I just tried it there are going to be quite a few patches.

I'm not sure how well shellcheck does with unconfigured cmake files but at a glance, the cmake variables don't seem to be causing a problem.

zapashcanon · 2017-04-15T22:50:31Z

@tresf, I fixed everything except one thing which seems strange to me.

We have:

STK_RAWWAVE=$HOME/stk-*/rawwaves

if [ ! -d "$STK_RAWWAVE" ]; then
   STK_RAWWAVE=$(brew --prefix stk)/share/stk/rawwaves
fi

Shellcheck doesn't like that. What's the * here for ? Is the folder name composed of some version number ? I did a quick fix with:

STK_RAWWAVE_LIST=( $HOME/stk-*/rawwaves )
STK_RAWWAVE=${STK_RAWWAVE_LIST[0]}

if [ ! -d "$STK_RAWWAVE" ]; then
   STK_RAWWAVE=$(brew --prefix stk)/share/stk/rawwaves
fi

But I don't know what's this code and I probably can't test it so...

tresf · 2017-04-15T23:54:16Z

@zapashcanon your assumptions are exactly correct. The purpose is a bit more vague... perhaps it could benefit from a comment but the raw waves must be copied at bundle time and at the time of writing that script, MacPorts didn't have libstk available.

Since then, we've moved to Homebrew (which does have libstk), so I kept the home-built rawwaves directory for backwards compatibility although I no longer use it for bundling and haven't tested against MacPorts in quite a while.

zapashcanon · 2017-04-16T17:16:14Z

Ok, so what should I do ? Is the current script ok ?

tresf · 2017-04-17T06:26:17Z

@zapashcanon sorry I was giving a background story on it all. It looks good. Merging. Note, to other devs... we will not be back-porting this to stable-1.2.

zapashcanon · 2017-04-17T14:31:02Z

Thanks for the review @tresf and @jasp00 ! :)

PhysSong · 2017-06-24T08:02:09Z

@tresf @jasp00 shellcheck now fails because the trusty image used by Travis CI got updated.
See this.

tresf · 2017-06-24T15:51:19Z

@zapashcanon spearheaded this PR, I'll see if he's interested in fixing it. Seems like the documentation is straightforward.

zapashcanon · 2017-06-25T20:14:46Z

I'll open a PR soon, should be easy.

Note: koalaman/shellcheck#942

Add shellcheck to the build

tresf reviewed Apr 1, 2017

View reviewed changes

jasp00 reviewed Apr 2, 2017

View reviewed changes

tresf reviewed Apr 10, 2017

View reviewed changes

jasp00 reviewed Apr 10, 2017

View reviewed changes

Add shellcheck to the build

7844b3e

tresf merged commit cea7d7d into LMMS:master Apr 17, 2017

zapashcanon deleted the shellcheck branch April 17, 2017 14:32

zapashcanon mentioned this pull request Jun 26, 2017

Fix scripts #3667

Merged

PhysSong mentioned this pull request Jul 8, 2017

Merge stable-1.2 into master #3607

Closed

tresf mentioned this pull request Oct 6, 2017

Add .clang-format file and format the src/ and include/ with it. #3843

Closed

PhysSong mentioned this pull request Mar 30, 2020

core: Fix controls linking misbehaviors (#4904) #5336

Merged

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

Merge pull request LMMS#3476 from zapashcanon/shellcheck

42449d3

Add shellcheck to the build

		@@ -49,4 +46,4 @@ notifications:
		- https://webhooks.gitter.im/e/1ac7fc698195981a9227


		export CMAKE_FLAGS="-DWANT_QT5=$QT5 -DUSE_WERROR=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo"

		if [ -z "$TRAVIS_TAG" ]; then export CMAKE_FLAGS="$CMAKE_FLAGS -DUSE_CCACHE=ON"; fi

Add shellcheck to the build #3476

Add shellcheck to the build #3476

Conversation

zapashcanon commented Apr 1, 2017

tresf Apr 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Apr 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Apr 1, 2017

zapashcanon commented Apr 2, 2017

tresf commented Apr 2, 2017

tresf commented Apr 2, 2017 • edited Loading

zapashcanon commented Apr 2, 2017

tresf commented Apr 2, 2017

zapashcanon commented Apr 2, 2017

tresf commented Apr 2, 2017

jasp00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapashcanon Apr 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapashcanon Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapashcanon Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapashcanon commented Apr 2, 2017

zapashcanon commented Apr 8, 2017

tresf commented Apr 8, 2017

zapashcanon commented Apr 9, 2017

jasp00 commented Apr 9, 2017

tresf commented Apr 10, 2017

tresf Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapashcanon commented Apr 10, 2017

jasp00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapashcanon Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Apr 1, 2017 •

edited

Loading

tresf Apr 2, 2017 •

edited

Loading

tresf commented Apr 2, 2017 •

edited

Loading

zapashcanon Apr 2, 2017 •

edited

Loading

zapashcanon Apr 8, 2017 •

edited

Loading

zapashcanon Apr 4, 2017 •

edited

Loading

tresf Apr 10, 2017 •

edited

Loading

zapashcanon Apr 10, 2017 •

edited

Loading

tresf commented Apr 14, 2017 •

edited

Loading

tresf commented Apr 15, 2017 •

edited

Loading