The linters are apparently breaking left and right on local machines and need to be disabled by developers. Ping @practicalswift et al.
I haven’t seen any information on this except a mention in IRC, so any hints or error logs on this would be appreciated.
“A new circular dependency in the form of “policy/policy -> validation -> pow -> policy/policy” appears to have been introduced. ^—- failure generated from test/lint/lint-circular-dependencies.sh The command “test/lint/lint-all.sh” exited with 1.”
0$ git diff master src/policy
1$
Sidenote: I confusedly thought I’d somehow #included validation.h and fixed the circular dependency. It then congratulated me, told me ANOTHER circular dependency had been introduced in policy/fees (which I haven’t touched either), and then failed again.
pow.cpp does not include policy/policy.h .
I think that the linter(lint-circular-dependencies.sh and circular-dependencies.py) treats *.cpp and *.h in the same way, and sometimes make a miss-judgement.
@kallewoof Thanks for the ping! Sounds bad. Let’s try to resolve that quickly!
Pinging in @Empact who wrote the linter for circular dependencies and @sipa who wrote the original Python script that the linter uses.
Please report any linter issues here!
I think that the linter(lint-circular-dependencies.sh and circular-dependencies.py) treats *.cpp and *.h in the same way, and sometimes make a miss-judgement.
This is true and I added an escape hatch for it in #10973: cbbb1d5cfb5fc47b84d6992aa6996b56f4b38a0d
@Empact This is not a circular inclusion.
I reproduced this error by specifying 3 cpp filenames to circular-dependencies.py on @kallewoof ’s signet branch as following:
0$ cd src
1$ ../contrib/devtools/circular-dependencies.py policy/policy.cpp validation.cpp pow.cpp
The linter issues seems limited to circular dependencies detection judging from the information in this PR?
If so the statement “linters are apparently breaking left and right on local machines and need to be disabled by developers” was a false alarm I guess? :-)
If there are any linter issues beyond the circular dependencies stuff I’d like to know about them so that I can fix them quickly! :-)
I’ve had to work around issues in my prs as new linters have been added.
It would be nice if before adding a new linter, you tested it on open PRs that have been updated recently, and notified failing PR authors so they could look into errors and fix things or report problems with the linter.
@ryanofsky Ah, just to clarify – so these were issues discovered by the linters rather than problems in the linters? :-) Have you encountered any issues in the linters themselves?
Yes, there is the bug in the circular dependencies linter described above: #14041 (comment), a bug in the format-strings linter fixed in #10443 (61ab4c17eebf9b73267c992fb664d603620f9e81), and a bug in include-guards linter fixed in #10102 (0f956d6b0d26c92a1656976b0518e0a469033e47).
But to me it is irrelevant whether bug is in the linter or not. For example, the linter bugs I found were pretty easy to fix, but I spent hours refactoring code and introducing a callback mechanism in pr/fee.18 (#10443) to avoid referencing CTxMemPoolEntry in fees_input.h and introducing a new circular dependency.
It would be nice to get warnings when a new linter is added that breaks PRs, so issues can be fixed in advance and don’t result in PRs being left conflicted while lint issues are debugged. Since linters are fast to run and don’t require building, it seems like it would be very easy to write a script that would check a new linter against a list of PRs and capture all the errors, so PR authors could have advance warning about problems.
@ryanofsky Yes, that would be the ideal way to do it when introducing new linters! I’ll look what I can do to make that happen :-)
When scripting that - should I perhaps fetch all PR:s without merge conflicts and build errors that have been updated during the N last days? If so, what would be an appropriate number for N? :-)