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

PEX files are directories instead of files #14980

Closed
asherf opened this issue Apr 1, 2022 · 4 comments · Fixed by #15014
Closed

PEX files are directories instead of files #14980

asherf opened this issue Apr 1, 2022 · 4 comments · Fixed by #15014
Assignees
Labels
Milestone

Comments

@asherf
Copy link
Member

asherf commented Apr 1, 2022

This is on latest master and on pants 2.11 rc1.
repo steps (in the pants repo): ./pants package src/python/pants/bin/::
Screen Shot 2022-04-01 at 10 32 14 AM

@asherf asherf added the bug label Apr 1, 2022
@stuhood stuhood added this to the 2.11.x milestone Apr 1, 2022
@stuhood
Copy link
Member

stuhood commented Apr 1, 2022

It sounds like the default value of the pex_binary layout= field might have been changed accidentally.

@stuhood stuhood self-assigned this Apr 1, 2022
@stuhood
Copy link
Member

stuhood commented Apr 1, 2022

I got distracted by another issue for which we only have an internal repro, so am looking at that first. I've tagged this for 2.11.x.

@stuhood stuhood removed their assignment Apr 1, 2022
@jsirois
Copy link
Contributor

jsirois commented Apr 2, 2022

Some research (The obvious likely commit is #14944):

At the commit before:

if request.internal_only or is_monolithic_resolve:
# This is a much friendlier layout for the CAS than the default zipapp.
layout = PexLayout.PACKED
else:
layout = request.layout or PexLayout.ZIPAPP
argv.extend(["--layout", layout.value])

And, higher up:

if isinstance(request.requirements, (Lockfile, LockfileContent)):
is_monolithic_resolve = True
resolve_name = request.requirements.resolve_name

It looks like that leads to this new code in #14944:
https://github.com/pantsbuild/pants/pull/14944/files#diff-796c32e79f63384b20df5eaaf3add13cff74e21470a478675503a5bf10bf77c9R377

@thejcannon & @Eric-Arellano I'll defer to you two to dive further on a fix for this and move on to triaging other issues. If I clear my deck I'll circle back and take a stab if neither of you have.

@stuhood
Copy link
Member

stuhood commented Apr 4, 2022

This will likely be affected by #14991 as well, so maybe I should take it back on. Thanks John.

@stuhood stuhood self-assigned this Apr 4, 2022
stuhood added a commit to stuhood/pants that referenced this issue Apr 6, 2022
stuhood added a commit that referenced this issue Apr 6, 2022
…15014)

As reported in #14980 and #14991: 1) the default layout of non-internal PEXes changed to `PACKED`, and 2) the PEX-repository subsetting optimization was disabled for manual or poetry lockfiles.

This change fixes those issues, and adds a test of the `pex_from_targets` codepath which decides which resolution strategy to use. But it also fairly significantly refactors the `PexRequest` consumption code to improve typesafety. In particular:
* Replaces the `is_all_constraints_resolve` special case with explicitly specified `PexLayout`s for those `PexRequests` (which would have been harder at the time when the special case was originally added, since `PexRequest.layout` didn't exist yet).
* Moves lockfile header parsing and PEX-native-detection into a `@rule` which produces a `LoadedLockfile`.
* Moves "subsetting of a lockfile or a repository PEX" to the `PexRequirements.from_subset` field, which takes a `LoadedLockfile` and asserts (not worth introducing another type yet probably) that the lockfile is PEX-native.
* Move the requirements which used to be attached to the `Lockfile`/`LockfileContents` off onto an `PexRequest.requirements = EntireLockfile` case, to assert that they are optional, and only used for metadata validation (rather than subsetting).

Fixes #14980 and fixes #14991.
stuhood added a commit to stuhood/pants that referenced this issue Apr 7, 2022
…antsbuild#15014)

As reported in pantsbuild#14980 and pantsbuild#14991: 1) the default layout of non-internal PEXes changed to `PACKED`, and 2) the PEX-repository subsetting optimization was disabled for manual or poetry lockfiles.

This change fixes those issues, and adds a test of the `pex_from_targets` codepath which decides which resolution strategy to use. But it also fairly significantly refactors the `PexRequest` consumption code to improve typesafety. In particular:
* Replaces the `is_all_constraints_resolve` special case with explicitly specified `PexLayout`s for those `PexRequests` (which would have been harder at the time when the special case was originally added, since `PexRequest.layout` didn't exist yet).
* Moves lockfile header parsing and PEX-native-detection into a `@rule` which produces a `LoadedLockfile`.
* Moves "subsetting of a lockfile or a repository PEX" to the `PexRequirements.from_subset` field, which takes a `LoadedLockfile` and asserts (not worth introducing another type yet probably) that the lockfile is PEX-native.
* Move the requirements which used to be attached to the `Lockfile`/`LockfileContents` off onto an `PexRequest.requirements = EntireLockfile` case, to assert that they are optional, and only used for metadata validation (rather than subsetting).

Fixes pantsbuild#14980 and fixes pantsbuild#14991.
# 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 issue Apr 7, 2022
…cherrypick of #15014) (#15032)

As reported in #14980 and #14991: 1) the default layout of non-internal PEXes changed to `PACKED`, and 2) the PEX-repository subsetting optimization was disabled for manual or poetry lockfiles.

This change fixes those issues, and adds a test of the `pex_from_targets` codepath which decides which resolution strategy to use. But it also fairly significantly refactors the `PexRequest` consumption code to improve typesafety. In particular:
* Replaces the `is_all_constraints_resolve` special case with explicitly specified `PexLayout`s for those `PexRequests` (which would have been harder at the time when the special case was originally added, since `PexRequest.layout` didn't exist yet).
* Moves lockfile header parsing and PEX-native-detection into a `@rule` which produces a `LoadedLockfile`.
* Moves "subsetting of a lockfile or a repository PEX" to the `PexRequirements.from_subset` field, which takes a `LoadedLockfile` and asserts (not worth introducing another type yet probably) that the lockfile is PEX-native.
* Move the requirements which used to be attached to the `Lockfile`/`LockfileContents` off onto an `PexRequest.requirements = EntireLockfile` case, to assert that they are optional, and only used for metadata validation (rather than subsetting).

Fixes #14980 and fixes #14991.

[ci skip-rust]
[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants