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

Fix poetry_requirements: ignore internal projects. #12280

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Jul 5, 2021

Previously dependencies with paths were assumed to be non-Pants
controlled projects and the resulting requirements were not generated
with absolute file URLs. Fix both issues by assuming all paths that
point to directories inside the build root are Pants controlled
projects and treating the rest as absolute file URL requirements.

Fixes #12272

[ci skip-rust]
[ci skip-build-wheels]

Move to parse state storage from ParseContext to parser where it belongs.
This cleans up the conceptual model a bit and allows for typing without
untoward dependencies.

# 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]
Previously dependencies with paths were assumed to be non-Pants
controlled projects and the resulting requirements were not generate
with absolute file URLs. Fix both issues by assuming all paths that
point to directories inside the build root are Pants controlled
projects and treating the rest as absolute file URL requirements.

Fixes pantsbuild#12272

[ci skip-rust]
[ci skip-build-wheels]
@jsirois
Copy link
Contributor Author

jsirois commented Jul 5, 2021

Depends on #12279. Please review the second commit (b7deacb) and forward.

@jsirois
Copy link
Contributor Author

jsirois commented Jul 5, 2021

Obviously resolve and is_dir are not awesome, but we're in a macro so we can do this. The solution is to kill macros and replace them with rules that generate Targets which would be better on several levels including automatically handling invalidation based on the underlying file changing (pyproject.toml, requirements.txt, etc.).

# 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]
@stuhood stuhood added this to the 2.6.x milestone Jul 6, 2021
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.

Thank you!

Comment on lines +143 to +154
"""Determine if the given path represents a non-Pants controlled project.

If the path points to a file, it's assumed the file is a distribution ( a wheel or sdist)
and the absolute path of that file is returned.

If the path points to a directory and that directory is outside of the build root, it's
assumed the directory is the root of a buildable Python project (i.e.: it contains a
pyproject.toml or setup.py) and the absolute path of the project is returned.

Otherwise, `None` is returned since the directory lies inside the build root and is assumed
to be a Pants controlled project.
"""
Copy link
Member

Choose a reason for hiding this comment

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

The resolution of filesystem paths makes this more susceptible to #7022, but I don't see a good way to resolve that without resolving the underlying issue.

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, John. This looks great.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit b73fe28 into pantsbuild:main Jul 7, 2021
Eric-Arellano pushed a commit to Eric-Arellano/pants that referenced this pull request Jul 7, 2021
Previously dependencies with paths were assumed to be non-Pants
controlled projects and the resulting requirements were not generated
with absolute file URLs. Fix both issues by assuming all paths that
point to directories inside the build root are Pants controlled
projects and treating the rest as absolute file URL requirements.

Fixes pantsbuild#12272

[ci skip-rust]
[ci skip-build-wheels]
Eric-Arellano added a commit that referenced this pull request Jul 7, 2021
@jsirois jsirois deleted the issues/12272/omit-local-project-dependencies branch July 12, 2021 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[poetry] Cannot depend on relative paths (non-local file URIs are not supported on this platform)
3 participants