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.
Concept ACK
Concept ACK
Very good idea!
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.
Could squash d3d19e39111f5a2b7318b8e80a1fbf61f3bb4f99 and d913a4b into 67eee3cc56e6b8014b73c07d8a064b5731a8239e
0 | @@ -0,0 +1,25 @@ 1 | +#!/usr/bin/env bash 2 | +# 3 | +# Copyright (c) 2017 The Bitcoin Core developers
2018?
fixed
Concept ACK
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
@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 d55d49c0f9c672327c4eea87aaa7b0f03381eebb
Now with the build steps numbered I moved the remaining steps too in a8803bd9e64e6416e6012c22eee849a20a64f633
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
26 | @@ -27,15 +27,15 @@ env: 27 | - WINEDEBUG=fixme-all 28 | - DOCKER_PACKAGES="build-essential libtool autotools-dev automake pkg-config bsdmainutils curl git ca-certificates ccache" 29 | before_install: 30 | - - source .travis/test_03_before_install.sh 31 | + - set -e; source .travis/test_03_before_install.sh
nit: How about set -o errexit as more self-documenting?
<!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:
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.
rebased.
in between #13859 was merged, I'm now invoking python with LC_ALL=C.UTF_8 in a31bdfcb918469c9d0400a2bd8433fbfddcc411b as it requires that to interpret the emoji correctly. or parse the source file in the first place. Otherwise nothing changed.
5 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php. 6 | + 7 | +export LC_ALL=C 8 | + 9 | +travis_retry docker pull "$DOCKER_NAME_TAG" 10 | +env | grep -E '^(CCACHE_|WINEDEBUG|LC_ALL|BOOST_TEST_RANDOM|CONFIG_SHELL)' | tee /tmp/env
It is somewhat confusing to set LC_ALL=C.UTF-8 in the travis yaml, then set it to LC_ALL=C here, save it into a file, read that by docker, but then overwrite it again with LC_ALL=C.UTF-8 for the test runner.
The lint-shell-locale.sh requires scripts to set LC_ALL=C. I could change it to also accept LC_ALL=C.UTF-8.
@MarcoFalke I've changed the scripts to export LC_ALL=C.UTF-8 and allowed that in lint-shell-locale in 50a51061fd04549625dedeed02bdf890548e3506 (rebased, now 54949499c1fdb860a6019d0b213b187e51a0a1b6)
utACK
16 | @@ -16,7 +17,7 @@ for SHELL_SCRIPT in $(git ls-files -- "*.sh" | grep -vE "src/(secp256k1|univalue 17 | continue 18 | fi 19 | FIRST_NON_COMMENT_LINE=$(grep -vE '^(#.*|)$' "${SHELL_SCRIPT}" | head -1) 20 | - if [[ ${FIRST_NON_COMMENT_LINE} != "export LC_ALL=C" ]]; then 21 | + if [[ ${FIRST_NON_COMMENT_LINE} != "export LC_ALL=C" && ${FIRST_NON_COMMENT_LINE} != "export LC_ALL=C.UTF-8" ]]; then
nit: if [[ ! ${FIRST_NON_COMMENT_LINE} =~ "export LC_ALL=C"(|\.UTF-8)$ ]]; then
I prefer the current version. It allows for copy and paste.
what about
case "${FIRST_NON_COMMENT_LINE}" in
"export LC_ALL=C");;
"export LC_ALL=C.UTF-8");;
*)
echo "Missing \"export LC_ALL=C\" (to avoid locale dependence) as first non-comment non-empty line in ${SHELL_SCRIPT}"
EXIT_CODE=1
esac
LGTM
pushed 336b9ae61511275570bebf4797343f6973c5e9a3
@scravy Oh, I think a simple if-statement is much clearer than the suggested case construction. I suggest reverting back to the original version which was perfectly fine and simple :-)
🧐removed the commit again.
@scravy Looks good now! Keep it that way :-)
utACK a8bf3c5a70e9924fd967d62a25915b68d2a6b0bc
About conflicting pull requests:
This one I just rebased.
Can this be merged or are there any more remarks/suggestions?
@scravy It needs review and testing
So I was wondering about that. If something touches the travis-definitions only – what is the difference between ACK and utACK here, what could I do to test it? I am rebasing this PR every day and the travis build works splendidly every time (except for this one time where everything in travis failed as it did not at all have any network connectivity).
Because I do not see what more I could do (as a reviewer) but look at travis whether it worked or not. Possibly I could imagine forking and pushing it in my own github-space and looking at travis for my repo, seeing if I can make it fail in the way it should fail (again, as a reviewer; obviously I did that but I'm the author).
I'm mostly asking out of curiosity.
Travis is completely useless when it passes every time. It must also detect any issues that are present and report them correctly.
Obviously it passes every time because there is no other development happening on this branch, and I'm rebasing against things which passed the CI check before. While reacting to change requests in the discussion in here it very well failed a few times.
For easier verification would it make sense to link these failing builds?
Again though, what is a reviewer supposed to do in order to test this? The only thing I can imagine is push it from your own repository to trigger a travis build for that and try to make it break there, right?
@scravy Yes, a good start would be your idea to try to cause each script to fail individually and verify that the build fails as expected. For the linting script - introduce a linting violation that should make it fail, etc :-)
adjust indentation to 2 spaces
Rebased.
In https://github.com/bitcoin/bitcoin/pull/13863/commits/8ad042b4bebbfd2e38bb4cc7ed36bc047aaf09cd 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: <img width="1570" alt="screen shot 2018-08-27 at 14 32 16" src="https://user-images.githubusercontent.com/295504/44659955-493c1480-aa06-11e8-9bbd-bcd0c7ab499f.png"> <img width="1557" alt="screen shot 2018-08-27 at 14 31 57" src="https://user-images.githubusercontent.com/295504/44659959-4c370500-aa06-11e8-8730-a27035ba4c4a.png">
In https://github.com/bitcoin/bitcoin/pull/13863/commits/236091eebfa9fb0234b0ff7549ec730e87d5a589 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 )
<img width="1583" alt="screen shot 2018-08-27 at 14 43 39" src="https://user-images.githubusercontent.com/295504/44660343-9cfb2d80-aa07-11e8-955a-f12e5ec6cda6.png"> <img width="1543" alt="screen shot 2018-08-27 at 14 42 38" src="https://user-images.githubusercontent.com/295504/44660304-7fc65f00-aa07-11e8-8646-a3ccb3f7f379.png">
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 )
<img width="1551" alt="screen shot 2018-08-27 at 14 44 47" src="https://user-images.githubusercontent.com/295504/44660445-ddf34200-aa07-11e8-8c92-8eda08dcde86.png"> <img width="1548" alt="screen shot 2018-08-27 at 14 46 46" src="https://user-images.githubusercontent.com/295504/44660501-0c711d00-aa08-11e8-8984-e4c052d70694.png">
In https://github.com/bitcoin/bitcoin/pull/13863/commits/b1ab0435ad245161c3a4906b0fe4a68bd211927a 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
<img width="1553" alt="screen shot 2018-08-27 at 14 55 42" src="https://user-images.githubusercontent.com/295504/44660888-4f7fc000-aa09-11e8-9323-c3981a2faacc.png"> <img width="1545" alt="screen shot 2018-08-27 at 14 55 33" src="https://user-images.githubusercontent.com/295504/44660889-4f7fc000-aa09-11e8-8eb3-78b43cceb22d.png"> <img width="1548" alt="screen shot 2018-08-27 at 14 54 20" src="https://user-images.githubusercontent.com/295504/44660891-4f7fc000-aa09-11e8-9622-51236f9adf51.png"> <img width="1554" alt="screen shot 2018-08-27 at 14 54 10" src="https://user-images.githubusercontent.com/295504/44660893-4f7fc000-aa09-11e8-93e0-90ccd1430bec.png">
42 | + 43 | +BEGIN_FOLD build 44 | +DOCKER_EXEC make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && DOCKER_EXEC make $GOAL V=1 ; false ) 45 | +END_FOLD 46 | + 47 | +if [ "$RUN_TESTS" = "true" ]; then
There is no symbol with that name, please adjust
58 | + 59 | +if [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then 60 | + extended="--extended --exclude feature_pruning,feature_dbcrash" 61 | +fi 62 | + 63 | +if [ "$RUN_TESTS" = "true" ]; then
Same here
@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.