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

Add tab-completion support to PEX repls. #2321

Merged
merged 6 commits into from
Jan 15, 2024

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Jan 11, 2024

This works for all supported Python versions even when those versions
own REPL does not support tab-completion out of the box.

@jsirois jsirois force-pushed the repl/support-completion branch 2 times, most recently from 9a13dbb to 4ad367e Compare January 14, 2024 19:52
@@ -234,37 +235,95 @@ def test_pex_repl_built():
assert b">>>" in stdout


@pytest.mark.skipif(
IS_PYPY or IS_MAC,
reason="REPL history is only supported on CPython. It works on macOS in an interactive "
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 appears to never have been true. In fact, using PEX_INTERPRETER_HISTORY / PEX_INTERPRETER_HISTORY_FILE on any Python interpreter without readline support, even CPython, previously led to:

$ PEX_INTERPRETER_HISTORY=1 PEX_INTERPRETER_HISTORY_FILE=/tmp/check $PWD/install/bin/python3.12 empty.pex
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/__main__.py", line 106, in <module>
    bootstrap_pex(__entry_point__, execute=__execute__, venv_dir=__venv_dir__)
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/.bootstrap/pex/pex_bootstrapper.py", line 627, in bootstrap_pex
    pex.PEX(entry_point).execute()
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/.bootstrap/pex/pex.py", line 561, in execute
    sys.exit(self._wrap_coverage(self._wrap_profiling, self._execute))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/.bootstrap/pex/pex.py", line 468, in _wrap_coverage
    return runner(*args)
           ^^^^^^^^^^^^^
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/.bootstrap/pex/pex.py", line 499, in _wrap_profiling
    return runner(*args)
           ^^^^^^^^^^^^^
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/.bootstrap/pex/pex.py", line 585, in _execute
    return self.execute_interpreter()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jsirois/.pex/unzipped_pexes/e2c4c91895f250a4f203b1d575cb3c39208ea5a7/.bootstrap/pex/pex.py", line 661, in execute_interpreter
    import readline
ModuleNotFoundError: No module named 'readline'

(Here I hand-built CPython3.12 with ./configure --prefix $PWD/install --without-readline)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, archaeology on #2018 (whose CI logs have unfortunately expired) finds a commit message stating that the issue on pypy (in CI) was apparently related to terminfo, so may have been due to interaction between pypy and the CI environment rather than pypy per se, but AFAICT wasn't due to unavailability of readline. Similarly, the macos ENOTTY issue only manifested in CI, and was not related to the presence of readline.

Anyway, if this is now deemed false and the actual underlying issue is just readline then the mentions of CPython in pex/variables.py#531 and pex/variables.py#542 should be removed.

Copy link
Member Author

@jsirois jsirois Jan 15, 2024

Choose a reason for hiding this comment

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

Yeah, definitely false. CI passes for pypy3.10 and mac and I've personally tested for pypy{2.7,3.{5,6,7,8,9,10}}. This PR had to plumb TERM to tests to work around 1/2 of the tty issues (using pexpect / ptys was the other half).

Variables docs fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I should have dived deeper in #2018 then, sounds like TERM would have made things work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, you'd have still needed to use psuedo terminals for the notty issue as in this PR and then figured out the libedit _HiStOrY_V2_ magic to work around error 22 on mac. It was a full 3 days of swearing for me anyway to move from works on my machine to fully tested.

This works for all supported CPython versions, although automated
testing is limited to CPython 3.6+.
Copy link
Collaborator

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nifty!

)
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

python has -S that (among other things) disables completion. It looks like this is unconditional, if the modules exist.

Is there potential risks/downsides to doing this unconditionally without a knob to turn it off, beyond the (seemingly small) performance cost of this set-up?

I assume that it's harmless enough, just double checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I got familiar with the CPython site.py code that -S disables in order to get this all working. I can't see any harm.

@jsirois jsirois merged commit 23d4810 into pex-tool:main Jan 15, 2024
26 checks passed
@jsirois jsirois deleted the repl/support-completion branch January 15, 2024 07:30
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.

3 participants