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

Use regex-lint to provide circular dependency checking in pants #5778

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

cognifloyd
Copy link
Member

Background

This is another part of introducing pants, as discussed in the TSC Meetings on 12 July 2022, 02 Aug 2022, 06 Sept 2022, and 04 Oct 2022. Pants has fine-grained per-file caching of results for lint, fmt (like black), test, etc. It also has lockfiles that work well for monorepos that have multiple python packages. With these lockfiles CI should not break when any of our dependencies or our transitive dependencies release new versions, because CI will continue to use the locked version until we explicitly relock with updates.

To keep PRs as manageable/reviewable as possible, introducing pants will take a series of PRs. I do not know yet how many PRs; I will break this up into logical steps with these goals:

  • introduce pants to the st2 repo, and
  • teach some (but not all) TSC members about pants step-by-step.

Other pants PRs include:

Overview of this PR

The Makefile has some checks for circular dependencies. Pants has a builtin regex-lint subsystem that lets us do basically the same thing via pants.

These are the relevant targets in the Makefile:

st2/Makefile

Lines 578 to 591 in 94d0798

.PHONY: .st2client-dependencies-check
.st2client-dependencies-check:
@echo "Checking for st2common imports inside st2client"
find ${ROOT_DIR}/st2client/st2client/ -name \*.py -type f -print0 | xargs -0 cat | grep st2common ; test $$? -eq 1
.PHONY: .st2common-circular-dependencies-check
.st2common-circular-dependencies-check:
@echo "Checking st2common for circular dependencies"
find ${ROOT_DIR}/st2common/st2common/ -name \*.py -type f -print0 | xargs -0 cat | grep st2reactor ; test $$? -eq 1
find ${ROOT_DIR}/st2common/st2common/ \( -name \*.py ! -name runnersregistrar\.py -name \*.py ! -name compat\.py ! -name inquiry\.py \) -type f -print0 | xargs -0 cat | grep st2actions ; test $$? -eq 1
find ${ROOT_DIR}/st2common/st2common/ -name \*.py -type f -print0 | xargs -0 cat | grep st2api ; test $$? -eq 1
find ${ROOT_DIR}/st2common/st2common/ -name \*.py -type f -print0 | xargs -0 cat | grep st2auth ; test $$? -eq 1
find ${ROOT_DIR}/st2common/st2common/ \( -name \*.py ! -name router\.py -name \*.py \) -type f -print0 | xargs -0 cat | grep st2stream; test $$? -eq 1
find ${ROOT_DIR}/st2common/st2common/ -name \*.py -type f -print0 | xargs -0 cat | grep st2exporter; test $$? -eq 1

It turns out that the Makefile targets are somewhat out-of-date: For example, some of the listed exceptions to the rules no longer apply and we removed st2exporter in #5676. I documented each of my updates in comments of the lint config file.

In #5776, I had to add our st2flake8 plugin for flake8 which handles checking for the copyright. We can replace that plugin with regex-lint. I noted how we can do that in the regex-lint config file, but I left it commented out for now. We can enable it later when we're ready to retire the st2flake8 custom plugin, which we can probably do once we're ready to remove the Makefile and related files.

Relevant Pants documentation

Things you can do with pantsbuild

You can run ./pants lint :: to see if regex-lint finds any issues (the GHA Lint workflow runs this as well). You can run only one of the linters with the helpful --only arg like this:

$ ./pants lint --only=regex-lint ::
13:21:52.61 [INFO] Initializing scheduler...
13:21:52.93 [INFO] Scheduler initialized.
13:21:54.27 [INFO] Completed: Lint with regex patterns - regex-lint succeeded.
385 files matched all required patterns.
0 files failed to match at least one required pattern.



✓ regex-lint succeeded.

The --only flag is "scoped" to the lint goal. So, these are equivalent if you wanted to specify that flag in a different order:
./pants lint --only=regex-lint ::
./pants --lint-only=regex-lint lint ::
./pants lint :: --lint-only=regex-lint

note: that I had to add lint in the arg name to use a different order

@cognifloyd cognifloyd added this to the pants milestone Oct 15, 2022
@cognifloyd cognifloyd self-assigned this Oct 15, 2022
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Oct 15, 2022
Copy link
Member

@rush-skills rush-skills left a comment

Choose a reason for hiding this comment

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

LGTM


- name: must_not_import_st2common
pattern: st2common
inverted: true
Copy link
Member

Choose a reason for hiding this comment

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

What does inverted mean here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Search for st2common but mark files as successful if they do NOT have the string.

@cognifloyd cognifloyd deleted the pants-regex-lint branch November 29, 2022 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure: ci/cd pantsbuild size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants