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

Do not use a repository-PEX if a PEX has platforms specified #15031

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Apr 6, 2022

#14995 shows that #12222 re-broke for the enable_resolves case, since the fix for the issue was only checking the resolve_all_constraints field. Fixes #14995.

@stuhood stuhood added the category:bugfix Bug fixes for released features label Apr 6, 2022
"enable_resolves",
[
False,
pytest.param(True, marks=pytest.mark.xfail(reason="#14991 causes this to fail in 2.11.x")),
Copy link
Member Author

Choose a reason for hiding this comment

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

This test (which asserts that we don't use the entire lockfile) cannot pass on main due to #14991, but I've validated that it passes on 2.10.x, and will remove the xfail while picking it.

@stuhood stuhood added this to the 2.10.x milestone Apr 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.

Thank you!

[ci skip-build-wheels]
[ci skip-rust]
@stuhood stuhood force-pushed the stuhood/do-not-use-repository-pex-if-platforms branch from 2a0f650 to ff263be Compare April 7, 2022 00:06
@stuhood stuhood enabled auto-merge (squash) April 7, 2022 00:06
Comment on lines +180 to +182
constraints_strings = FrozenOrderedSet(
str(req) for req in parse_requirements_file(lock_string, rel_path=lockfile_path)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this will handle hash correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed: added another commit to improve parsing.

[ci skip-rust]
[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]
[ci skip-rust]

[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/do-not-use-repository-pex-if-platforms branch from ff263be to 2f6f8e6 Compare April 7, 2022 02:29
@stuhood stuhood merged commit a8d851c into pantsbuild:main Apr 7, 2022
@stuhood stuhood deleted the stuhood/do-not-use-repository-pex-if-platforms branch April 7, 2022 03:17
@Eric-Arellano
Copy link
Contributor

Parsing algorithm looks good. Thanks for adding tests!

stuhood added a commit to stuhood/pants that referenced this pull request Apr 7, 2022
stuhood added a commit to stuhood/pants that referenced this pull request Apr 7, 2022
…ild#15031)

pantsbuild#14995 shows that pantsbuild#12222 re-broke for the `enable_resolves` case, since the fix for the issue was only checking the `resolve_all_constraints` field. Fixes pantsbuild#14995.
# 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 added a commit that referenced this pull request Apr 7, 2022
…ick of #15031) (#15034)

#14995 shows that #12222 re-broke for the `enable_resolves` case, since the fix for the issue was only checking the `resolve_all_constraints` field. Fixes #14995.

[ci skip-rust]
[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Apr 7, 2022
…ick of #15031) (#15033)

#14995 shows that #12222 re-broke for the `enable_resolves` case, since the fix for the issue was only checking the `resolve_all_constraints` field. Fixes #14995.
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.

Failure to build an AWS Lambda on macOS, even without native deps
3 participants