Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 7 commits into from
Aug 27, 2018
Merged

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

maflcko merged 7 commits into from
Aug 27, 2018

Conversation

@scravy
Copy link
Contributor

@scravy scravy 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 mentioned this pull request Aug 3, 2018
Closed
@scravy scravy changed the title Travis extract scripts travis: move script sections to files in .travis/ subject to shellcheck Aug 3, 2018
@Empact
Copy link
Contributor

Empact commented Aug 3, 2018

Concept ACK

@practicalswift
Copy link
Contributor

practicalswift commented Aug 3, 2018

Concept ACK

Very good idea!

@maflcko
Copy link
Member

maflcko 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
Copy link
Contributor

Empact commented Aug 3, 2018

Could squash d3d19e39111f5a2b7318b8e80a1fbf61f3bb4f99 and d913a4b into 67eee3cc56e6b8014b73c07d8a064b5731a8239e

ken2812221
ken2812221 suggested changes Aug 3, 2018
View reviewed changes
Copy link
Contributor

@ken2812221 ken2812221 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

.travis/before_install.sh Outdated
Copy link
Contributor

@ken2812221 ken2812221 Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018?

Copy link
Contributor Author

@scravy scravy Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@scravy scravy force-pushed the travis-extract-scripts branch from d913a4b to 8644fe8 Compare August 3, 2018 15:09
@maflcko
Copy link
Member

maflcko 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 travis-extract-scripts branch from 8644fe8 to b8d52be Compare August 3, 2018 15:17
@scravy
Copy link
Contributor Author

scravy commented Aug 3, 2018

@Empact squashed and reordered the commits a bit

@scravy
Copy link
Contributor Author

scravy commented Aug 3, 2018 •
edited
Loading

@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 travis-extract-scripts branch from 7c020f7 to a8803bd Compare August 3, 2018 15:35
@scravy
Copy link
Contributor Author

scravy commented Aug 3, 2018

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

@Empact
Copy link
Contributor

Empact 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
Copy link
Contributor Author

scravy commented Aug 3, 2018

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

Empact
Empact reviewed Aug 3, 2018
View reviewed changes
.travis.yml Outdated
Copy link
Contributor

@Empact Empact Aug 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@scravy scravy force-pushed the travis-extract-scripts branch 2 times, most recently from e20473b to 5d920b5 Compare August 3, 2018 16:26
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 3, 2018 •
edited
Loading

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.

This was referenced Aug 3, 2018
Merged
Closed
Closed
Merged
Merged
@scravy scravy force-pushed the travis-extract-scripts branch from 5d920b5 to c235fc3 Compare August 6, 2018 09:11
@scravy scravy mentioned this pull request Aug 6, 2018
Merged
@scravy scravy force-pushed the travis-extract-scripts branch 2 times, most recently from b3bbf53 to 3b996c2 Compare August 6, 2018 14:43
@str4d str4d mentioned this pull request Oct 27, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Oct 28, 2020
@zkbot
Auto merge of #4829 - str4d:backport-lints, r=<try>
43ac206 
Backport useful lints from upstream

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6892
- bitcoin/bitcoin#11151
- bitcoin/bitcoin#11300
- bitcoin/bitcoin@96d91b7
- bitcoin/bitcoin#12097
- bitcoin/bitcoin#12098
- bitcoin/bitcoin#12442
- bitcoin/bitcoin#12572
- bitcoin/bitcoin#12757
- bitcoin/bitcoin#11878
- bitcoin/bitcoin#12933
- bitcoin/bitcoin#12871
- bitcoin/bitcoin#12972
- bitcoin/bitcoin#13281
- bitcoin/bitcoin#13385
- bitcoin/bitcoin#13041
- bitcoin/bitcoin#13454
- bitcoin/bitcoin#13448
- bitcoin/bitcoin#13510
- bitcoin/bitcoin#13851
- bitcoin/bitcoin#13863
- bitcoin/bitcoin#14115
- bitcoin/bitcoin#14831
- bitcoin/bitcoin#15164
- bitcoin/bitcoin#15170
- bitcoin/bitcoin#15166
- bitcoin/bitcoin#16036
- bitcoin/bitcoin#16768

Several of the lints fail for our current codebase; these will be addressed in a
subsequent PR.
zkbot added a commit to zcash/zcash that referenced this pull request Nov 9, 2020
@zkbot
Auto merge of #4829 - str4d:backport-lints, r=str4d
84a5830 
Backport useful lints from upstream

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6892
- bitcoin/bitcoin#11151
- bitcoin/bitcoin#11300
- bitcoin/bitcoin@96d91b7
- bitcoin/bitcoin#12097
- bitcoin/bitcoin#12098
- bitcoin/bitcoin#12442
- bitcoin/bitcoin#12572
- bitcoin/bitcoin#12757
- bitcoin/bitcoin#11878
- bitcoin/bitcoin#12933
- bitcoin/bitcoin#12871
- bitcoin/bitcoin#12972
- bitcoin/bitcoin#13281
- bitcoin/bitcoin#13385
- bitcoin/bitcoin#13041
- bitcoin/bitcoin#13454
- bitcoin/bitcoin#13448
- bitcoin/bitcoin#13510
- bitcoin/bitcoin#13851
- bitcoin/bitcoin#13863
- bitcoin/bitcoin#14115
- bitcoin/bitcoin#14831
- bitcoin/bitcoin#15164
- bitcoin/bitcoin#15170
- bitcoin/bitcoin#15166
- bitcoin/bitcoin#16036
- bitcoin/bitcoin#16768
- bitcoin/bitcoin#13494

Several of the lints fail for our current codebase; these will be addressed in a
subsequent PR.
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 7, 2021
@UdjinM6
Partial merge bitcoin#13863: travis: move script sections to files in…
77bbcd9 
… `.travis/` subject 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 bitcoin#13816 to make that one easier to review. It follows on bitcoin#13849 and bitcoin#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
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 8, 2021
@Munkybooty
Partial merge bitcoin#13863: travis: move script sections to files in…
4c2211d 
… `.travis/` subject 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 bitcoin#13816 to make that one easier to review. It follows on bitcoin#13849 and bitcoin#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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@Empact Empact Empact left review comments

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift left review comments

@ken2812221 ken2812221 ken2812221 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@scravy @Empact @practicalswift @maflcko @DrahtBot @Gnappuraz @ken2812221 @fanquake