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

Gracefully degrade when loading libreadline fails #807

Closed
wants to merge 2 commits into from

Conversation

jayvdb
Copy link

@jayvdb jayvdb commented Nov 15, 2019

libreadline may not be loadable for many reasons, especially
in environments where a packager is using zipapp or PyOxidizer.
Avoid critical failure; degrade gracefully to operating without
libreadline and notify the user this has happened.

Related to #802

libreadline may not be loadable for many reasons, especially
in environments where a packager is using zipapp or PyOxidizer.
Avoid critical failure; degrade gracefully to operating without
libreadline and notify the user this has happened.

Related to python-cmd2#802
@jayvdb jayvdb requested a review from kmvanbrunt as a code owner November 15, 2019 06:37
@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #807 into master will decrease coverage by 0.1%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #807      +/-   ##
=========================================
- Coverage    97.3%   97.2%   -0.11%     
=========================================
  Files          14      14              
  Lines        3493    3501       +8     
=========================================
+ Hits         3399    3403       +4     
- Misses         94      98       +4
Impacted Files Coverage Δ
cmd2/rl_utils.py 89.36% <63.63%> (-8.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf4d87...4b0254f. Read the comment docs.

@jayvdb
Copy link
Author

jayvdb commented Nov 15, 2019

Let me know if I need to build convoluted tests to reach these.

import ctypes
readline_lib = ctypes.CDLL(readline.__file__)
try:
import ctypes
Copy link
Member

Choose a reason for hiding this comment

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

Why would an import of ctypes fail?

Copy link
Author

Choose a reason for hiding this comment

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

ctypes is a module. It doesnt need to exist, and is one of the first to be disabled in embedded Python, especially on new arch/platforms where the entire gcc toolchain isnt available/stable yet. In a completely static build of Python with all needed modules built in, there is no need for ctypes, and it adds bloat.

@kmvanbrunt
Copy link
Member

We are comfortable with cmd2 working in typical desktop environments running standard Python distributions as well as in containers like docker. If we choose to add exception handling to this code later, one of our authors will do it.

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.

2 participants