add shellcheck test, fix code for shellcheck #8

Merged
merged 25 commits into from Apr 28, 2017

Conversation

Projects
None yet
4 participants
Contributor

ogra1 commented Apr 25, 2017

  • sets up xenial env
  • runs shellcheck
  • builds two source packages
  • builds two binary packages
.travis.yml
@@ -0,0 +1,24 @@
+language: C
+dist: xenial
@zyga

zyga Apr 25, 2017

Contributor

Is that a real thing now? I thought travis just "upgraded" defaults to 14.04 but only supported precise and trusty.

@ogra1

ogra1 Apr 25, 2017

Contributor

well, it isnt ... but it is also ignored and describes that we now use a xenial chroot inside when testing ;)

-if [ "$(stat -c %s $FILE)" -ne "$((1024*1024*$SIZE))" ]; then
- delete_swapfile $FILE
- create_swapfile $FILE $SIZE
+if [ "$(stat -c %s "$FILE")" -ne "$((1024*1024*SIZE))" ]; then
@zyga

zyga Apr 25, 2017

Contributor

Wow that's some pretty weird syntax.

@ogra1

ogra1 Apr 25, 2017

Contributor

it is comparing byte sizes ... which part do you find weird

else
# we are not at the end ! get the start of the next partition
# (minus 1 byte) instead of using the absolute end of the disk
- endsize=$(($(parted -ms $DEV unit B print|grep ^$(($num+1)):|\
@zyga

zyga Apr 25, 2017

Contributor

So $num was incorrect here? Just checking

@ogra1

ogra1 Apr 25, 2017

Contributor

yes, $num never existed, and likewise we do not have any image yet where the "else" would actually be in effect (only if "writable" is not the last partition on disk, we do not have such images yet)

Looks reasonable, although I'm not aware of the details of the actual tests run.

.travis.yml
+script:
+ - sudo chroot $CHROOT sh -c 'cd build; for file in $(find . | xargs file | grep shell |grep -v .git| sed "s/:.*$/ /g" | tr -d "\n"); do shellcheck $SHCKOPTS $file; done'
+ # test build src packages unsigned in a xenial chroot
+ - sudo chroot xenial-test-chroot sh -c 'cd build/initramfs; LC_ALL=C.UTF-8 dpkg-buildpackage -rfakeroot -S -sa -us -uc'
@niemeyer

niemeyer Apr 27, 2017

Why above uses $CHROOT but these don't?

The common prefix here might also be turned into a variable btw ($CHROOT_RUN or something).

@ogra1

ogra1 Apr 27, 2017

Contributor

nice catch! i was testing the line locally and forgot to substitute the variable back in when i copy/pasted it after testing...

substitute all chroot call with a variable
- fix accidentially hardcoded chroot names
- switch to CHROOT_RUN  variable for actual chroot invocation

zyga approved these changes Apr 28, 2017

All the shellcheck-driven changes look correct. I didn't dig into the travis testing but I trust it works.

}
do_mbr()
{
DEV=$1
PART=$2
- endsize=$(get_end $PART)
- parted -s $DEV resizepart $PART $endsize
+ endsize=$(get_end "$PART")
@zyga

zyga Apr 28, 2017

Contributor

You can also quote the outer part but I think it's actually not required here.

@mvo5 mvo5 merged commit a647c65 into snapcore:master Apr 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment