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

pants: register requirements for contrib/examples pack #5834

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

cognifloyd
Copy link
Member

Background

This is another part of introducing pants, as discussed in various TSC meetings.

Related PRs can be found in:

Overview of this PR

In #5733, I configured pants to ignore several requirements files, including contrib/examples/requirements.txt:

st2/pants.toml

Lines 28 to 29 in f25df1f

# keep tailor from using legacy requirements files (not for pants)
"contrib/examples/requirements.txt",

One of the reasons I excluded that file, beyond what I described in #5733, was because it includes requests which is already included in the main requirements files:


requests[security]

If we included both of these, then pants would start complaining that import requests is ambiguous all over the place. Compare that to #5833 where I had to disambiguate some other dependencies, but those affected a more isolated set of files. We do not want to register explicit dependencies on requests all over the place as that would be quite painful. So, excluding the requirements file was the best course of action.

However, there are two more dependencies that are NOT in the main requirements file:

beautifulsoup4
flask

These deps, beautifulsoup4 and flask, are only needed for the examples pack. Including those in the main requirements file would be a signal for future contributors that they can use those in other parts of the codebase, but we don't want anything other than the examples pack to depend on them.

So, this PR:

This PR also shows an example of how github displays the lockfile diff when we add new dependencies.

Aside: Once we upgrade to pants 2.16 (which is in development), a new "dependency rules" feature will allow us to enforce that nothing else can import these deps. That will make this even safer.

Relevant Pants documentation

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

Note: This PR needs to be merged before I can add pylint. Otherwise, pants will run into these ambiguous deps whenever it runs pylint, and pylint will subsequently fail when pants doesn't add all of the required deps to the sandbox it runs pylint in.

@cognifloyd cognifloyd enabled auto-merge (squash) December 3, 2022 19:10
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

LGTM

@cognifloyd cognifloyd requested a review from a team December 5, 2022 15:56
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

@cognifloyd cognifloyd merged commit 0fc59f7 into master Dec 6, 2022
@cognifloyd cognifloyd deleted the pants-ambiguous-reqs branch December 6, 2022 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependency 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.

3 participants