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 the Pex CLI to work when installed read-only. #2205

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Aug 3, 2023

When Pex is run from a read-only environment such as a venv that has
been made read-only after the fact or a nix environment, it must not
attempt to over-write its own vendored .bootstrap code in the process
of creating PEX runtime environments. Fix .bootstrap creation to only
write bootstrap files once.

Fixes #2203

When Pex is run from a read-only environment such as a venv that has
been made read-only after the fact or a nix environment, it must not
attempt to over-write its own vendored `.bootstrap` code in the process
of creating PEX runtime environments. Fix `.bootstrap` creation to only
write bootstrap files once.

Fixes pex-tool#2203
abs_path = os.path.join(root, path)
os.chmod(abs_path, os.stat(abs_path).st_mode & ~write_mask)

assert_pex_works()
Copy link
Member Author

@jsirois jsirois Aug 3, 2023

Choose a reason for hiding this comment

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

This 2nd assert_pex_works() failed in the same manner as described in #2203 and NixOS/nixpkgs#246448 prior to the fix.

@@ -591,9 +591,11 @@ def _prepare_bootstrap(self):
dirs[:] = bootstrap_packages

for f in filter_pyc_files(files):
if f.endswith("testing.py"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right. You reviewed that one actually in #2197. I noticed this miss in that PR while I was in this code.

@jsirois jsirois merged commit e8c0085 into pex-tool:main Aug 3, 2023
@jsirois jsirois deleted the issues/2203 branch August 3, 2023 22:54
Copy link
Contributor

@cosmicexplorer cosmicexplorer left a comment

Choose a reason for hiding this comment

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

non-blocking comment, thanks for including me!

Comment on lines +595 to +598
# N.B.: Some of the `pex.*` package files (__init__.py) will already have been
# prepared when vendoring the runtime above; so we skip them here.
if abs_src in prepared_sources:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular part regarding files that "will already have been prepared" and how it relates to writing files into the read-only directory is confusing to me and I would appreciate a quick note on how we avoid writing to a read-only dir with this deduplication process if you have a moment.

This exact line/if check is the part that fixes #2203, right? I'm confused as to why not writing duplicate files (which clearly seems good, and probably improves performance too) is fixing the issue with writing to a read-only directory.

  • I only see mutating a single Chroot instance here, which I assume is a directory we're allowed to write to.
  • I see that we reference _ABS_PEX_PACKAGE_DIR in this method, which I assume is the bootstrap directory (which may be read-only) that we were previously writing files to, but I don't see how we were writing any files to it.
  • I also see that vendored_sources stops us from writing the same files to the chroot, which is another deduplication; but that just looks like an optimization (and when I remove it, the new test still passes)?

Thanks for your time, but no pressure to respond, as this is mostly out of curiosity.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm; found the reasoning at #2203! sorry for ping

The issue here is that Pex trips over itself when constructing the .bootstrap by attempting to copy pex/init.py and other root pex.* package init.py files multiple times. The 1st copy works, but since the source file is read only, the destination file is as well, and subsequent copies (over-writes), fail.

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.

The Pex CLI does not work when run from a read-only venv.
3 participants