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

tailor should skip bad requirements targets #15755

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Jun 6, 2022

Adds basic validation for requirements.txt, pipfile, and pyproject.toml by tailor so that we don't permanently start trying to ingest invalid files.

Closes #15734

Christopher Neugebauer added 4 commits June 3, 2022 14:03
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn added the category:bugfix Bug fixes for released features label Jun 6, 2022
Christopher Neugebauer added 5 commits June 6, 2022 12:32
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn changed the title [15734] tailor should skip bad requirements targets tailor should skip bad requirements targets Jun 6, 2022
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks!

The rule was already probably too big, and this makes it even bigger. I recommend factoring out a @rule_helper specifically for 3rd-party requirements. Grep for rule_helper to see examples of how it works.

f"No target generated for `{fp}`. You'll need to create "
"targets for its contents manually."
)
logger.warning("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to have a newline? We have no precedent for this, and imo it's confusing to see WARN twice.

If you really wanted a newline, it's better to use \n in the call above. But again, no precedent for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, probably better to combine the warning with the implementation-specific warnings. We try to have logs be self-contained

src/python/pants/backend/python/goals/tailor_test.py Outdated Show resolved Hide resolved
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! One comment.

src/python/pants/backend/python/goals/tailor.py Outdated Show resolved Hide resolved
@chrisjrn
Copy link
Contributor Author

chrisjrn commented Jun 6, 2022

I'll add in @stuhood's suggestions, and come back to @Eric-Arellano's as a follow-up

@Eric-Arellano
Copy link
Contributor

and come back to @Eric-Arellano's as a follow-up

Other than @rule_helper, I think our feedback was the same.

Christopher Neugebauer added 2 commits June 6, 2022 14:57
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn
Copy link
Contributor Author

chrisjrn commented Jun 6, 2022

Yup, it's the @rule_helper that I'll deal with later

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

f"An error occurred when validating `{fp}`: {e}.\n\n"
"You'll need to create targets for its contents manually.\n"
"To silence this error in future, see "
"https://www.pantsbuild.org/docs/reference-tailor#section-ignore-paths \n"
Copy link
Member

Choose a reason for hiding this comment

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

Should use from pants.util.docutil import doc_url for this.

logger.warning(
f"An error occurred when validating `{fp}`: {e}.\n\n"
"You'll need to create targets for its contents manually.\n"
"To silence this error in future, see "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"To silence this error in future, see "
"To silence this warning in future, see "

Copy link
Contributor

Choose a reason for hiding this comment

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

"the future"

Comment on lines +181 to +182
for _ in parse_requirements_file(contents.decode(), rel_path=path):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for _ in parse_requirements_file(contents.decode(), rel_path=path):
pass
parse_requirements_file(contents.decode(), rel_path=path):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a generator, we need to consume its output somehow

@chrisjrn chrisjrn merged commit 5e208ec into pantsbuild:main Jun 7, 2022
@chrisjrn chrisjrn deleted the chrisjrn/15734-tailor-skip-bad-requirements-targets branch June 7, 2022 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tailor should not create targets for unparseable requirements.txt/pyproject.toml/etc.
3 participants