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

Enable SC2185 again #4022

Merged
merged 3 commits into from
Feb 7, 2019
Merged

Enable SC2185 again #4022

merged 3 commits into from
Feb 7, 2019

Conversation

zapashcanon
Copy link
Contributor

@zapashcanon zapashcanon commented Nov 29, 2017

Edit: @tresf, repurposed this PR.

Original:

Title: Enable SC2185 again

Fix #3684.

If it doesn't pass now, we will have to wait until a more recent version of ShellCheck is available in the CI.

Reusing this PR to shellcheck all lmms directories up to 3 directories deep.

@PhysSong
Copy link
Member

Travis-CI still seems to use ShellCheck 0.46 now. I think we should wait for next Travis-CI image update.

@tresf
Copy link
Member

tresf commented Nov 30, 2017

shellcheck 0.4.6 is available yet shellcheck 0.3.3 is what Ubuntu 14.04 uses. We should leverage a more modern version using some other technique. Perhaps the docker technique explained here? https:/koalaman/shellcheck/wiki/TravisCI

Or is there a modern PPA available?

@zapashcanon
Copy link
Contributor Author

I don't know, currently the style build is used only for SC, but if we wan't to add stuff later, it may not be a good idea to use the SC docker image... Maybe using something like a Debian Unstable docker image which would allow us to have recent version of many things could be something to try ? (Currently, SC is 0.4.6 in Debian Unstable too... but, should be updated quickly I hope)

@tresf
Copy link
Member

tresf commented Nov 30, 2017

@zapashcanon I vote we close this out for now. We can always follow up with it with it when our build system gets updated. @lukas-w has a PR open for switching us to Circle-CI/docker images but I don't see a benefit in keeping it open.

@zapashcanon
Copy link
Contributor Author

I don't know, I'd maybe rather leave it open so I don't forget it should be fixed as I often check my opened PR list here ; and I'll try to relaunch the build sometimes to see if SC has been updated.

@tresf
Copy link
Member

tresf commented Dec 1, 2017

so I don't forget it should be fixed as I often check my opened PR list here ; and I'll try to relaunch the build sometimes to see if SC has been updated.

Normally I'd agree.... but in this case I think it's smarter to wait until the CI environment changes as Ubuntu 14.04 seems to have no intentions of updating this package and a change to the CI environment requires this PR to be rebased anyway, which seems like overkill for a one-liner.

@tresf
Copy link
Member

tresf commented Dec 1, 2017

Actually... perhaps we can have the latest using PPA "pinning". https://help.ubuntu.com/community/PinningHowto

@tresf
Copy link
Member

tresf commented Dec 2, 2017

Ok... here's how we pin it from the future 18.04. I've tested this on 14.04 and seems to work just fine.

sudo add-apt-repository "deb http://us.archive.ubuntu.com/ubuntu/ bionic universe"
sudo sh -c 'echo  "Package: shellcheck\nPin: release n=bionic\nPin-Priority: 999\n" > /etc/apt/preferences.d/ubuntu-bionic-pin-999'

# update, install
sudo apt-get update
sudo apt-get install shellcheck

shellcheck --version
# ShellCheck - shell script analysis tool
# version: 0.4.6
# license: GNU General Public License, version 3
# website: http://www.shellcheck.net

Note, for small utilities like shellcheck, this is rather harmless however the pinning guide cautions that for larger ones like gcc, qt5, etc. we should continue to stick to a community recommended and tested 14.04 PPA back-port.

@tresf
Copy link
Member

tresf commented Dec 24, 2017

@zapashcanon please switch this PR to use a modern mirror (such as through PPA pinning) or close it out.

Added via d7aeee0.

Edit: Reverted d7aeee0, Ubuntu 18.04 still doesn't have the right version a year later. 😢

@tresf
Copy link
Member

tresf commented Feb 7, 2019

Ugh... SC2185 has bad code. I'll quote the wiki:

### Exceptions:

You will get a false positive if you concatenate a series of pre-path flags:

find -XLE .

In such cases, please either use find -X -L -E . or [[ignore]] the message.

Since -O3 is a single command (not a combination of commands), this is a false-positive. I guess we'll have to explicitly ignore it. The irony because it's the shellcheck script itself. 🌠

Edit: Nevermind, a year later, I'm rediscovering the original bug due to 18.04 still not having the updated version.

@tresf
Copy link
Member

tresf commented Feb 7, 2019

Ok, I'm stumbling on the original bug. Turns out, even Ubuntu 18.04 doesn't ship with shellcheck 0.4.7 yet (it's at 0.4.6) which is required for this patch. It's been over a year and no Ubuntu LTS distro supplies this. I'm going to choose to ignore this. The code is self-documenting and leaving the PR open until the next LTS doesn't make sense.

As an aside, the bash completion does't pass shellcheck but it's ignored because we only scan two directories. Changing to scan all directories with a -maxdepth of 3 to avoid traversing into plugin directories.

@tresf tresf merged commit 7a0b874 into LMMS:master Feb 7, 2019
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.

3 participants