lint: Convert lint-circular-dependencies.sh to Python by Smlep · Pull Request #24915 · bitcoin/bitcoin

Smlep · 2022-04-18T22:59:37Z

Here is a port of /test/lint/lint-circular-dependencies.sh to a Python-script as part of the request of #24783.

It aims to provide the same output as the bash version.

brunoerg · 2022-04-19T00:31:38Z

I think you could squash the commits since they're related.

DrahtBot · 2022-04-19T01:55:25Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

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.

Smlep · 2022-04-19T06:44:42Z

I think you could squash the commits since they're related.

It's done @brunoerg 👍

KevinMusgrave · 2022-04-19T21:33:01Z

Tested 23e047620f85636195c8d7b81891ec3a8693d671. The new version could maybe use a line break before the Good job message.

As a test I added:

#include <index/coinstatsindex.h> to index/blockfilterindex
#include <index/blockfilterindex.h> to index/coinstatsindex

Before (test/lint/lint-circular-dependencies.sh):

A new circular dependency in the form of "index/blockfilterindex -> index/coinstatsindex -> index/blockfilterindex" appears to have been introduced.

A new circular dependency in the form of "index/blockfilterindex -> index/coinstatsindex -> validation -> index/blockfilterindex" appears to have been introduced.

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

After (test/lint/lint-circular-dependencies.py):

A new circular dependency in the form of "index/blockfilterindex -> index/coinstatsindex -> index/blockfilterindex" appears to have been introduced.
A new circular dependency in the form of "index/blockfilterindex -> index/coinstatsindex -> validation -> index/blockfilterindex" appears to have been introduced.
Good job! The circular dependency "index/blockfilterindex -> node/blockstorage -> validation -> index/blockfilterindex" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in test/lint/lint-circular-dependencies.py
to make sure this circular dependency is not accidentally reintroduced.

Sidenote: I'm not sure why adding a circular dependency causes an existing circular dependency to disappear

Smlep · 2022-04-19T21:41:49Z

@KevinMusgrave Indeed, the newline after each message from the original script was missing.
A new version has been pushed with this fix.

KevinMusgrave · 2022-04-19T21:52:15Z

Tested ACK ca52afd92be695b05fb13fe4f96a25ff2b3c7f9c

Output:

A new circular dependency in the form of "index/blockfilterindex -> index/coinstatsindex -> index/blockfilterindex" appears to have been introduced.

A new circular dependency in the form of "index/blockfilterindex -> index/coinstatsindex -> validation -> index/blockfilterindex" appears to have been introduced.

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

maflcko · 2022-04-20T08:04:38Z

test/lint/lint-circular-dependencies.py

any reason to ignore errors?

Suggested change

dependencies_output = subprocess.run(

command,

stdout=subprocess.PIPE,

dependencies_output = subprocess.check_output(

command,

Because ../contrib/devtools/circular-dependencies.py does not exit with 0 if there is at least one circular dependency, resulting in subprocess.check raising an error

test/lint/lint-circular-dependencies.py

.

laanwj · 2022-04-25T17:36:56Z

Tested ACK 79635c7

maflcko · 2022-04-26T10:09:07Z

test/lint/lint-circular-dependencies.py

+
+    # Using glob.glob since subprocess.run's globbing won't work without shell=True
+    files = []
+    for path in ["*", "*/*", "*/*/*"]:


Wouldn't it be better to call git ls-files -- '*.h' '*.cpp'?

maflcko · 2022-04-26T10:13:56Z

test/lint/lint-circular-dependencies.py

+        for extension in ["h", "cpp"]:
+            files.extend(glob.glob(f"{path}.{extension}"))
+
+    command = ["python3", "../contrib/devtools/circular-dependencies.py", *files]


Wouldn't it be better to use sys.executable instead of "python3"?

…ython 79635c7 lint: Convert lint-circular-dependencies.sh to Python (Smlep) Pull request description: Here is a port of `/test/lint/lint-circular-dependencies.sh` to a Python-script as part of the request of bitcoin#24783. It aims to provide the same output as the bash version. ACKs for top commit: laanwj: Tested ACK 79635c7 Tree-SHA512: f18077018f1229dd933cfe2bf0cfe7dc7d6538961c96a83c7a1f05e0cec4b068ca05502d68410d2aa4b6864523424db386e38233735190525904c2a8e9d2ba13

Smlep mentioned this pull request Apr 18, 2022

Port lint scripts to Python #24783

Closed

25 tasks

DrahtBot added the Tests label Apr 19, 2022

This was referenced Apr 19, 2022

refactor: Split ArgsManager out of util/system #24455

Closed

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

DrahtBot mentioned this pull request Apr 20, 2022

Improve Indices on pruned nodes via prune blockers #21726

Merged

maflcko previously requested changes Apr 20, 2022

View reviewed changes

DrahtBot mentioned this pull request Apr 20, 2022

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

lint: Convert lint-circular-dependencies.sh to Python

79635c7

laanwj merged commit 16fa967 into bitcoin:master Apr 25, 2022

maflcko reviewed Apr 26, 2022

View reviewed changes

bitcoin locked and limited conversation to collaborators Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: Convert lint-circular-dependencies.sh to Python#24915

lint: Convert lint-circular-dependencies.sh to Python#24915
laanwj merged 1 commit intobitcoin:masterfrom
Smlep:lint-circular-dependencies-port

Smlep commented Apr 18, 2022

Uh oh!

brunoerg commented Apr 19, 2022

Uh oh!

DrahtBot commented Apr 19, 2022 •

edited

Loading

Uh oh!

Smlep commented Apr 19, 2022

Uh oh!

KevinMusgrave commented Apr 19, 2022 •

edited

Loading

Uh oh!

Smlep commented Apr 19, 2022

Uh oh!

KevinMusgrave commented Apr 19, 2022

Uh oh!

maflcko Apr 20, 2022

Uh oh!

Smlep Apr 20, 2022 •

edited

Loading

Uh oh!

Uh oh!

laanwj commented Apr 25, 2022

Uh oh!

maflcko Apr 26, 2022

Uh oh!

maflcko Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

-    dependencies_output = subprocess.run(
-        command,
-        stdout=subprocess.PIPE,
+    dependencies_output = subprocess.check_output(
+        command,

Conversation

Smlep commented Apr 18, 2022

Uh oh!

brunoerg commented Apr 19, 2022

Uh oh!

DrahtBot commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Smlep commented Apr 19, 2022

Uh oh!

KevinMusgrave commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Smlep commented Apr 19, 2022

Uh oh!

KevinMusgrave commented Apr 19, 2022

Uh oh!

maflcko Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

Smlep Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Apr 25, 2022

Uh oh!

maflcko Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Apr 19, 2022 •

edited

Loading

KevinMusgrave commented Apr 19, 2022 •

edited

Loading

Smlep Apr 20, 2022 •

edited

Loading