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

lint: Make all linters work under the default macOS dev environment (build-osx.md) #14115

Merged
merged 4 commits into from Sep 5, 2018

Conversation

Projects
None yet
6 participants
@practicalswift
Copy link
Member

commented Aug 30, 2018

The linters are thoroughly tested under Ubuntu which is what we use in Travis. When reading #14041 I understood that some developers were experiencing problems when running the linters on their local machines.

Assuming these local machines were running macOS I installed a fresh macOS VM, followed the instructions in build-osx.md and ran the linters.

This PR contains the changes needed to make lint-all.sh run as expected.

Ideally the linters would continuously run also under a Travis macOS environment to make sure we catch these kind of issues before merge.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

Concept ACK

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

utACK 341f7c7

@Empact

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

See also: #13728 which is similar but fixes the linter errors rather than disabling them, and runs the linters on macos via CI.

@fanquake

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

Tested on macOS 10.13.6, master 2070a54 has a bunch of warnings/failures:

./test/lint/lint-all.sh
test/lint/lint-format-strings.sh: line 36: mapfile: command not found
grep: empty (sub)expression
/usr/local/lib/python3.7/site-packages/pycodestyle.py:113: FutureWarning: Possible nested set at position 1
  EXTRANEOUS_WHITESPACE_REGEX = re.compile(r'[[({] | []}),;:]')
grep: empty (sub)expression
Missing "export LC_ALL=C" (to avoid locale dependence) as first non-comment non-empty line in .travis/lint_04_install.sh
grep: empty (sub)expression
^---- failure generated from test/lint/lint-shell-locale.sh

Using 341f7c7 and after brew install spellcheck and pip3 install flake8, all linters seem to run, with no warnings.

Did not check if this breaks anything on a different OS etc.
tACK 341f7c7

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

Please review :-)

@MarcoFalke MarcoFalke merged commit 341f7c7 into bitcoin:master Sep 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 5, 2018

Merge #14115: lint: Make all linters work under the default macOS dev…
… environment (build-osx.md)

341f7c7 macOS fix: Check for correct version of flake8 to avoid spurious warnings. The brew installed flake8 version is Python 2 based and does not work. (practicalswift)
908a559 macOS fix: Add excludes for checks added in the newer shellcheck version installed by brew (practicalswift)
ec4d57b macOS fix: Work around empty (sub)expression error when using BSD grep (practicalswift)
b57d7d9 macOS fix: Avoid mapfile due to ancient version of bash shipped with macOS (practicalswift)

Pull request description:

  The linters are thoroughly tested under Ubuntu which is what we use in Travis. When reading #14041 I understood that some developers were experiencing problems when running the linters on their local machines.

  Assuming these local machines were running macOS I installed a fresh macOS VM, followed the instructions in `build-osx.md` and ran the linters.

  This PR contains the changes needed to make `lint-all.sh` run as expected.

  Ideally the linters would continuously run also under a Travis macOS environment to make sure we catch these kind of issues before merge.

Tree-SHA512: b39c9a970d14d27db1fb592539923c0bc676b5217f415d02fda3f17bf54d46faa172376e8a3ecab07ca68a3acba9aebe00b2b1b2161b2a36b85fbb672e7efb5c
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.