-
Notifications
You must be signed in to change notification settings - Fork 38.7k
lint: Run the CI lint stage on mac #13728
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
Conversation
|
Inspired by and built on #13720 |
052ed45 to
1598438
Compare
|
Concept ACK Nit: macOS ships with GNU bash, albeit an ancient version. Please change "helps ensure ongoing compatibility with BSD bash as well as GNU bash" to something along the lines of "helps ensure ongoing compatibility with the macOS BSD user land and the ancient version of GNU bash shipped with macOS". |
7436739 to
3a9c712
Compare
|
3a9c712 to
18591b2
Compare
|
|
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
18591b2 to
f94eafa
Compare
|
Might as well do a full depends and bitcoind build when pulling in macos anyway? |
3c87951 to
3d8c654
Compare
scravy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much like these changes - love the --exclude change, the -e made me go to the documentation the first time I came across it and I wished it was a more self-explanatory cli switch :-)
3d8c654 to
d5d7665
Compare
|
ACK d5d76655105d6d338dbc367ae0b54b7433ffeb38 |
|
utACK d5d7665 Closing my PR #13720 in favour of this one. |
|
There are some non-travis related changes here, which should be submitted independently, imo. I.e. all commits to b24ee76 might go to a separate pull. |
d5d7665 to
daef159
Compare
|
@MarcoFalke FYI those changes are required to make the lint stage pass when also run on Mac, so I included them to make this PR green. Will extract a second PR for them in case that's helpful. |
| Needs rebase |
ccb7ec1 to
f6e2b0d
Compare
8ee21a1 to
27acd37
Compare
ci/lint/04_install.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea to add a travis_retry here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style-nit: If you feel like running source ./ci/test/00_setup_env.sh, you can use ${CI_RETRY_EXE} instead, which should also work on non travis hosts (theoretically), obviously not yet in practise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added travis_retry and switched -s to --silent for clarity.
@MarcoFalke will plan on a follow-up converting the lint scripts: https://github.com/Empact/bitcoin/tree/travis-retry-lint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looks like setup_env is already running (because the before_install step is inherited.
27acd37 to
fe91b3a
Compare
This helps ensure ongoing compatibility with macOS-distributed version of GNU bash.
Declare and assign separately to avoid masking return values. https://github.com/koalaman/shellcheck/wiki/SC2155
Particularly `--with-pcre2` is needed to run `git grep --perl-regexp` in `test/link/check-doc.py`
fe91b3a to
cd82f75
Compare
|
I am going to merge this under the "experimental umbrella", as it might be needed for #16597 |
|
It takes about 10 minutes to boot the macOS on travis. If that is too long, we can revert this. ACK cd82f75 Show signature and timestampSignature: Timestamp of file with hash |
@MarcoFalke Any idea why it takes that long? |
because travis 🤷 |
cd82f75 lint: Install grep and git via brew on mac for --perl-regexp (Ben Woosley) eafa747 lint: Fix shellcheck SC2155 (Ben Woosley) 615ff4e lint: Run the linters against Mac OS on Travis (Ben Woosley) Pull request description: This helps ensure ongoing compatibility with macOS-distributed version of GNU bash. ACKs for top commit: MarcoFalke: ACK cd82f75 Tree-SHA512: 8d56d2303bbebedba8ea2291f4ab35b7fdf3245b7a4c3f04557eee4f19d83573798ad32facc92bfa060aaeb294e6d2c95e6d1c3b795fd7951dcf3aa1cccec107
|
The backlog for macOS can be seen here: https://www.traviscistatus.com/ note to myself: It failed here: https://travis-ci.org/bitcoin/bitcoin/jobs/595893060#L2470 |
|
I think it's bad that this change seems to now require scripted diffs to run in the macos unix environment. It makes me less inclined to write scripted diffs if I have to somehow get them them to work in macos when I don't use the operating system, and don't have the hardware, and all the unix tools are strange and old. Also, does this scripted-diff checking script itself not function on macos...? $ set -o errexit; source ./ci/lint/06_script.sh
sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
Error: missing script for: 3c5edd40ae63daee4de6c58ea4123c66be83119c
Failed
sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
Error: missing script for: 7e271ea765b731c59edbc62790f568af284c7800
Failed |
|
There are some tools / homebrew packages to make macOS scripts behave more like Linux. E.g. https://formulae.brew.sh/formula/gnu-sed |
|
Ok, then lets remove this and macOS users should use the gnu utils that are provided by brew |
This helps ensure ongoing compatibility with macOS-distributed version of GNU bash.