Skip to content

test: Make lint-includes.sh work from any directory #16768

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 1 commit into from
Sep 5, 2019
Merged

test: Make lint-includes.sh work from any directory #16768

maflcko merged 1 commit into from
Sep 5, 2019

Conversation

@kristapsk
Copy link
Contributor

@kristapsk kristapsk commented Aug 30, 2019

Before this change it works from root folder of bitcoin git repo, but if you do cd test/lint; ./test-includes.sh, you will have a lot of false positive messages like this:

Good job! The circular dependency "chainparamsbase -> util/system -> chainparamsbase" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
to make sure this circular dependency is not accidentally reintroduced.

Good job! The circular dependency "index/txindex -> validation -> index/txindex" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
to make sure this circular dependency is not accidentally reintroduced.

@fanquake fanquake added the Tests label Aug 30, 2019
@kristapsk kristapsk force-pushed the lint-includes-anydir branch from 56ab427 to ed628d6 Compare August 30, 2019 22:15
@kristapsk
Copy link
Contributor Author

kristapsk commented Aug 30, 2019

Changed push/popd to cd, Travis CI didn't like it.

In test/lint/lint-includes.sh line 15:
pushd "$(dirname $0)/../.." > /dev/null
^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
In test/lint/lint-includes.sh line 114:
popd > /dev/null
^--------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

@fanquake
Copy link
Member

fanquake commented Aug 31, 2019

Rebooted the test to be sure, and Travis still doesn't like it:

In test/lint/lint-includes.sh line 15:
cd "$(dirname $0)/../.." > /dev/null
^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
In test/lint/lint-includes.sh line 114:
cd - > /dev/null
^--------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
^--------------^ SC2103: Use a ( subshell ) to avoid having to cd back.
For more information:
  https://www.shellcheck.net/wiki/SC2164 -- Use 'cd ... || exit' or 'cd ... |...
  https://www.shellcheck.net/wiki/SC2103 -- Use a ( subshell ) to avoid havin...
^---- failure generated from test/lint/lint-shell.sh

@kristapsk kristapsk force-pushed the lint-includes-anydir branch from ed628d6 to 0182555 Compare August 31, 2019 11:44
@kristapsk
Copy link
Contributor Author

kristapsk commented Aug 31, 2019 •
edited
Loading

It wants the check of exist status of cd, should be fine now.

practicalswift
practicalswift reviewed Sep 3, 2019
View reviewed changes
@practicalswift
Copy link
Contributor

practicalswift commented Sep 3, 2019

Tested ACK 01825553fd1e10d95c8b1539b8d0245ede39fd74

Nice developer ergonomics fix!

@kristapsk, you might want to apply this fix to the other git based linters as well to allow for running them from any directory. Perhaps in a follow-up PR? :-)

These linters invoke git:

$ git grep -lE "[^a-z]git[^a-z]" -- test/lint/ ":(exclude)*.md"
test/lint/check-doc.py
test/lint/commit-script-check.sh
test/lint/extended-lint-cppcheck.sh
test/lint/git-subtree-check.sh
test/lint/lint-assertions.sh
test/lint/lint-filenames.sh
test/lint/lint-format-strings.sh
test/lint/lint-include-guards.sh
test/lint/lint-includes.sh
test/lint/lint-locale-dependence.sh
test/lint/lint-logs.sh
test/lint/lint-python-dead-code.sh
test/lint/lint-python-mutable-default-parameters.sh
test/lint/lint-python-utf8-encoding.sh
test/lint/lint-python.sh
test/lint/lint-qt.sh
test/lint/lint-rpc-help.sh
test/lint/lint-shebang.sh
test/lint/lint-shell-locale.sh
test/lint/lint-shell.sh
test/lint/lint-spelling.sh
test/lint/lint-tests.sh
test/lint/lint-whitespace.sh

@maflcko
Copy link
Member

maflcko commented Sep 3, 2019

I'd rather not change all the linter scripts to provide maximum flexibility. This is all code that needs to be reviewed and maintained. And changing one script to be flexible, but keeping the others doesn't really help.

So -0 from me.

@kristapsk
Copy link
Contributor Author

kristapsk commented Sep 3, 2019

So, as I understand, @MarcoFalke is against merging this, but @practicalswift things it's good idea to modify other scripts too. What do others think? I will not stand and fight for this too hard, does not seem to be of high importance, just think it's not good that script gives false positive outputs just because it's ran from the wrong directory.

@practicalswift
Copy link
Contributor

practicalswift commented Sep 3, 2019

@kristapsk @MarcoFalke Even if the other scripts are left unchanged I think this specific change is worth doing: lint-includes.sh gives very misleading output if not run from the repo root.

@maflcko
Copy link
Member

maflcko commented Sep 3, 2019

Ok, fair enough

@kristapsk kristapsk force-pushed the lint-includes-anydir branch from 0182555 to 9d518b5 Compare September 4, 2019 00:27
promag
promag reviewed Sep 4, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

ACK 9d518b5f0ffedc79b98dba7aed35637d11e7f736, tested.

@kristapsk kristapsk force-pushed the lint-includes-anydir branch from 9d518b5 to 490da63 Compare September 4, 2019 19:36
maflcko pushed a commit that referenced this pull request Sep 5, 2019
Merge #16768: test: Make lint-includes.sh work from any directory
761fe07 
490da63 Make lint-includes.sh work from any directory (Kristaps Kaupe)

Pull request description:

  Before this change it works from root folder of bitcoin git repo, but if you do `cd test/lint; ./test-includes.sh`, you will have a lot of false positive messages like this:
  ```
  Good job! The circular dependency "chainparamsbase -> util/system -> chainparamsbase" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  Good job! The circular dependency "index/txindex -> validation -> index/txindex" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  ```

Top commit has no ACKs.

Tree-SHA512: 07fa69cb2883181dcee922191acac4b242722eeb2916cdffdc7163421302b22f3c9525aaf4c754a9dba1c307032c05285e38191d5c6aabc894321f8a27bbceaa
@maflcko maflcko merged commit 490da63 into bitcoin:master Sep 5, 2019
@kristapsk kristapsk deleted the lint-includes-anydir branch September 7, 2019 18:34
@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.
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@PastaPastaPasta
Merge bitcoin#16768: test: Make lint-includes.sh work from any directory
92e18e9 
490da63 Make lint-includes.sh work from any directory (Kristaps Kaupe)

Pull request description:

  Before this change it works from root folder of bitcoin git repo, but if you do `cd test/lint; ./test-includes.sh`, you will have a lot of false positive messages like this:
  ```
  Good job! The circular dependency "chainparamsbase -> util/system -> chainparamsbase" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  Good job! The circular dependency "index/txindex -> validation -> index/txindex" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  ```

Top commit has no ACKs.

Tree-SHA512: 07fa69cb2883181dcee922191acac4b242722eeb2916cdffdc7163421302b22f3c9525aaf4c754a9dba1c307032c05285e38191d5c6aabc894321f8a27bbceaa
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 13, 2021
@PastaPastaPasta
Merge bitcoin#16768: test: Make lint-includes.sh work from any directory
d36d5ef 
490da63 Make lint-includes.sh work from any directory (Kristaps Kaupe)

Pull request description:

  Before this change it works from root folder of bitcoin git repo, but if you do `cd test/lint; ./test-includes.sh`, you will have a lot of false positive messages like this:
  ```
  Good job! The circular dependency "chainparamsbase -> util/system -> chainparamsbase" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  Good job! The circular dependency "index/txindex -> validation -> index/txindex" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  ```

Top commit has no ACKs.

Tree-SHA512: 07fa69cb2883181dcee922191acac4b242722eeb2916cdffdc7163421302b22f3c9525aaf4c754a9dba1c307032c05285e38191d5c6aabc894321f8a27bbceaa
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@promag promag promag left review comments

@practicalswift practicalswift practicalswift 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.

5 participants

@kristapsk @fanquake @practicalswift @maflcko @promag