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

travis: move script sections to files in `.travis/` subject to shellcheck #13863

Merged
merged 7 commits into from Aug 27, 2018

Conversation

Projects
None yet
8 participants
@scravy
Copy link
Contributor

commented Aug 3, 2018

This PR is extracted from #13816 to make that one easier to review. It follows on #13849 and #13851

In here the shell script parts from travis.yml are extracted into .travis/before_install.sh, .travis/install.sh, .travis/before_script.sh, .travis/script.sh, and .travis/lint.sh.

This has the benefit that test/lint/lint-shell.sh will also shellcheck these parts. Also it makes the individual script parts more readable.

@scravy scravy changed the title Travis extract scripts travis: move script sections to files in `.travis/` subject to shellcheck Aug 3, 2018

@Empact

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Concept ACK

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Concept ACK

Very good idea!

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

ACK, this way we could also reset old travis pull request runs, since it is less likely that the travis.yml changed in the meantime.

@fanquake fanquake added the Tests label Aug 3, 2018

@Empact

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Could squash d3d19e39111f5a2b7318b8e80a1fbf61f3bb4f99 and d913a4b into 67eee3cc56e6b8014b73c07d8a064b5731a8239e

@ken2812221
Copy link
Member

left a comment

Concept ACK

.travis/before_install.sh Outdated
@@ -0,0 +1,25 @@
#!/usr/bin/env bash
#
# Copyright (c) 2017 The Bitcoin Core developers

This comment has been minimized.

Copy link
@ken2812221

This comment has been minimized.

Copy link
@scravy

scravy Aug 3, 2018

Author Contributor

fixed

@scravy scravy force-pushed the scravy:travis-extract-scripts branch Aug 3, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

I wonder if it helps to prefix the scripts with two digits to clarify the order in which they are run (03_before_install, 04_...) https://docs.travis-ci.com/user/customizing-the-build/#the-build-lifecycle

@scravy scravy force-pushed the scravy:travis-extract-scripts branch to b8d52be Aug 3, 2018

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@Empact squashed and reordered the commits a bit

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@MarcoFalke I've changed the names of the files according to your suggestion and added a README with the rationale/link to the travis documentation in d55d49c

@scravy scravy force-pushed the scravy:travis-extract-scripts branch Aug 3, 2018

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

Now with the build steps numbered I moved the remaining steps too in a8803bd9e64e6416e6012c22eee849a20a64f633

@Empact

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Big issue with this PR: Travis - sections check the return code on a per-command basis. The newly sourced files do not, so failures are not detected unless you detect them in the script and propagate them to Travis.

E.g. see https://travis-ci.org/Empact/bitcoin/jobs/411794591 where I deleted a file to force the test/lint/git-subtree-check.sh src/secp256k1 lint to fail

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@Empact great catch, I added set -e before the source step. WDYT?

.travis.yml Outdated
@@ -27,15 +27,15 @@ env:
- WINEDEBUG=fixme-all
- DOCKER_PACKAGES="build-essential libtool autotools-dev automake pkg-config bsdmainutils curl git ca-certificates ccache"
before_install:
- source .travis/test_03_before_install.sh
- set -e; source .travis/test_03_before_install.sh

This comment has been minimized.

Copy link
@Empact

Empact Aug 3, 2018

Member

nit: How about set -o errexit as more self-documenting?

@scravy scravy force-pushed the scravy:travis-extract-scripts branch 2 times, most recently Aug 3, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13954 (lint: Check for common misspellings using codespell. Fix typos reported by codespell. by practicalswift)
  • #13845 (Include tinyformat as a subtree by Empact)
  • #13728 (Scripts and tools: Run the CI lint stage on mac and linux both by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@scravy scravy force-pushed the scravy:travis-extract-scripts branch to 4143269 Aug 27, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 27, 2018

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Rebased.

@scravy scravy force-pushed the scravy:travis-extract-scripts branch 4 times, most recently Aug 27, 2018

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

In 8ad042b I am explicitly breaking the build by introducing an undocumented argument, which is supposed to fail the lint stage.

Here's the build failing in the lint stage in my repository:
https://travis-ci.org/scravy/bitcoin/jobs/421068804#L575

Here's the build failing in the lint stage as a pull request:
https://travis-ci.org/bitcoin/bitcoin/jobs/421068842#L580

Screenshots of the failing build:
screen shot 2018-08-27 at 14 32 16
screen shot 2018-08-27 at 14 31 57

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

In 236091e I'm undoing the lint-error such that the lint stage passes again and introduce a compilation error in src/init.cpp to fail the build in the test stage.

Here's the build failing in the test stage in my repository:
https://travis-ci.org/scravy/bitcoin/builds/421075688 (exact failure: https://travis-ci.org/scravy/bitcoin/jobs/421075692#L2865 )

screen shot 2018-08-27 at 14 43 39

screen shot 2018-08-27 at 14 42 38

Here's the build failing in the test stage as a pull request:
https://travis-ci.org/bitcoin/bitcoin/builds/421075705 (exact failure: https://travis-ci.org/bitcoin/bitcoin/jobs/421075707#L3120 )

screen shot 2018-08-27 at 14 44 47

screen shot 2018-08-27 at 14 46 46

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

In b1ab043 I undo the compile error and introduce a lint violation insisde on of the travis script files (not using LC_ALL=c, violating lint-shell-locale).

This is how it's failing in my repository:
https://travis-ci.org/scravy/bitcoin/jobs/421082198#L579

This is how it's failing as a pull request:
https://travis-ci.org/bitcoin/bitcoin/jobs/421082215#L585

screen shot 2018-08-27 at 14 55 42

screen shot 2018-08-27 at 14 55 33

screen shot 2018-08-27 at 14 54 20

screen shot 2018-08-27 at 14 54 10

@MarcoFalke MarcoFalke force-pushed the scravy:travis-extract-scripts branch to 4143269 Aug 27, 2018

@MarcoFalke MarcoFalke merged commit 4143269 into bitcoin:master Aug 27, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Aug 27, 2018

Merge #13863: travis: move script sections to files in `.travis/` sub…
…ject to shellcheck

4143269 use export LC_ALL=C.UTF-8 (Julian Fleischer)
728c82d make script exit if a command fails (Julian Fleischer)
506890b move remaining travis build steps into individual files (Julian Fleischer)
272306e number .travis/ script according to build lifecycle and add README to explain (Julian Fleischer)
519e273 move lint stage up to resemble travis build ui (Julian Fleischer)
86d34f0 abort script in END_FOLD on non-zero exit code (Julian Fleischer)
4f2f88c move script sections info individual files and comply with shellcheck (Julian Fleischer)

Pull request description:

  This PR is extracted from #13816 to make that one easier to review. It follows on #13849 and #13851

  In here the shell script parts from `travis.yml` are extracted into `.travis/before_install.sh`, `.travis/install.sh`, `.travis/before_script.sh`, `.travis/script.sh`, and `.travis/lint.sh`.

  This has the benefit that `test/lint/lint-shell.sh` will also shellcheck these parts. Also it makes the individual script parts more readable.

Tree-SHA512: c497e1687ceb1c1d795de177d3fc35af908bc8e3f781a871afabdecf031e581d4db229290627249e35ef7c09952bc34884e4734ea91d40f57b4a9efb85bba2e3
DOCKER_EXEC make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && DOCKER_EXEC make $GOAL V=1 ; false )
END_FOLD

if [ "$RUN_TESTS" = "true" ]; then

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 27, 2018

Member

There is no symbol with that name, please adjust

extended="--extended --exclude feature_pruning,feature_dbcrash"
fi

if [ "$RUN_TESTS" = "true" ]; then

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 27, 2018

Member

Same here

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

@MarcoFalke I see two review comments of yours, yet the PR was merged. You say RUN_TESTS is not exported? Will send a follow-up PR then.

@scravy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Fixed in #14081

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Aug 27, 2018

Merge bitcoin#14081: travis: fix missing differentiation between UNIT…
… and FUNCTIONAL tests

c55c5f2 fix missing differentiation between UNIT and FUNCTIONAL tests in travis build (Julian Fleischer)

Pull request description:

  @MarcoFalke follow up to bitcoin#13863

  I must have missed the separation of `RUN_FUNCTIONAL_TESTS` and `RUN_UNIT_TESTS` when doing the rebase. Fixed the two places you mentioned accordingly.

Tree-SHA512: 43d14cb16fe72f77c5a142509fb59849e32b58a12565a752e8b4e36282eb74f796b97140d9a64e1ba0d0409d07107f77fd84aaddf87617470f19ff0dd332dd58

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Dec 3, 2018

Merge bitcoin#14231: travis: Save cache even when build or test fail
d3ecc3d travis: Save cache on build error (Chun Kuan Lee)

Pull request description:

  In current travis setup, the job will terminate immediately if an error occur. There is no chance to save the cache. This was accidentally introduced by bitcoin#13863. This PR is to fix the issue and travis would save cache on error.

  test for build error: https://travis-ci.org/bitcoin/bitcoin/builds/429172128

Tree-SHA512: fb8beb97928e10932c695d1884948bf8972a6501042d5212111fba1f258160d813a4c6cc72e9da78f2acd9518382c21943347b820d8e15b5eb874e7707c928b2

@sickpig sickpig referenced this pull request Jan 18, 2019

Merged

Improve travis caching #1559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.