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

Smlep commented at 10:59 PM on April 18, 2022: contributor

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 commented at 12:31 AM on April 19, 2022: member

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

DrahtBot added the label Tests on Apr 19, 2022

DrahtBot commented at 1:55 AM on April 19, 2022: member

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 commented at 6:44 AM on April 19, 2022: contributor

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

It's done @brunoerg 👍

KevinMusgrave commented at 9:33 PM on April 19, 2022: contributor

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 commented at 9:41 PM on April 19, 2022: contributor

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

KevinMusgrave commented at 9:52 PM on April 19, 2022: contributor

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.

in test/lint/lint-circular-dependencies.py:50 in ca52afd92b outdated

  45 | +            files.extend(glob.glob(f"{path}.{extension}"))
  46 | +
  47 | +    command = ["python3", "../contrib/devtools/circular-dependencies.py", *files]
  48 | +    dependencies_output = subprocess.run(
  49 | +        command,
  50 | +        stdout=subprocess.PIPE,

MarcoFalke commented at 8:04 AM on April 20, 2022:

any reason to ignore errors?

    dependencies_output = subprocess.check_output(
        command,

Smlep commented at 9:03 PM on April 20, 2022:

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

in test/lint/lint-circular-dependencies.py:81 in ca52afd92b outdated

  76 | +                f"Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in {__file__}",
  77 | +                file=sys.stderr,
  78 | +            )
  79 | +            print(
  80 | +                "to make sure this circular dependency is not accidentally reintroduced.\n",
  81 | +                file=sys.stderr,

MarcoFalke commented at 8:07 AM on April 20, 2022:

why are you printing to stderr?

Also the exit code is wrong.

Smlep commented at 8:53 PM on April 20, 2022:

FIxed

MarcoFalke dismissed

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

laanwj commented at 5:36 PM on April 25, 2022: member

Tested ACK 79635c79e0533aa7f2e2440515ad766f965343ba

laanwj merged this on Apr 25, 2022

laanwj closed this on Apr 25, 2022

in test/lint/lint-circular-dependencies.py:43 in 79635c79e0

  38 | +        CODE_DIR
  39 | +    )  # We change dir before globbing since glob.glob's root_dir option is only available in Python 3.10
  40 | +
  41 | +    # Using glob.glob since subprocess.run's globbing won't work without shell=True
  42 | +    files = []
  43 | +    for path in ["*", "*/*", "*/*/*"]:

MarcoFalke commented at 10:09 AM on April 26, 2022:

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

in test/lint/lint-circular-dependencies.py:47 in 79635c79e0

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

MarcoFalke commented at 10:13 AM on April 26, 2022:

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

sidhujag referenced this in commit d4371f17a5 on Apr 26, 2022

DrahtBot locked this on Apr 26, 2023