-
Notifications
You must be signed in to change notification settings - Fork 400
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
Support for exposing manual pages #1047
Conversation
- Can install and uninstall a simple package with manual pages on Linux. - Not tested on macOS or Windows. - Tests not updated yet.
A function call to expose_resources_globally with a duplicate argument.
src/pipx/commands/common.py
Outdated
exposed_man_paths = [] | ||
for i in range(1, 10): | ||
man_section = "man%d" % i | ||
exposed_man_paths = get_exposed_man_paths_for_package( | ||
venv.man_path / man_section, | ||
constants.LOCAL_MAN_DIR / man_section, | ||
man_pages, | ||
) | ||
exposed_man_pages = sorted( | ||
str(Path(p.parent.name) / p.name) for p in exposed_man_paths | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this kind of structure appears often, could you integrate the loop in and return the exposed_man_pages
from get_exposed_man_paths_for_package()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets slightly comlicated because get_exposed_man_paths_for_package
is called from multiple places, among which is _get_venv_resource_paths
. _get_venv_resource_paths
is currently called for each man<i>
directory separately. The reason is because it checks if a symlink can be created in the directory (can_symlink(local_resource_dir)
), which shouldn't be done for the main man directory (~/.local/share/man
), but for each man<i>
subdirectory. _get_venv_resource_paths
then calls get_exposed_man_paths_for_package
indirectly through _get_package_man_paths
. If the man_section loop is located in get_exposed_man_paths_for_package
, there will have to be another man_section loop in _get_venv_resource_paths
for the can_symlink
checks. It is of course possible, but there isn't a neat and simple way of getting rid of the loops.
Another option how to make it slightly nicer would be to add a constant such as MAN_SECTIONS = ["man%d" % i for i in range(1, 10)]
in constants.py
and then loop over the list in other places.
src/pipx/venv_inspect.py
Outdated
def get_man_pages(dist: metadata.Distribution, man_path: Path) -> List[str]: | ||
man_pages = [] | ||
for i in range(1, 10): | ||
man_section = "man%d" % i | ||
names = get_resources("man", dist, man_path / man_section) | ||
for name in names: | ||
man_pages = [str(Path(man_section) / name)] | ||
return man_pages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to merge this with get_resources()
and return a nested list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I have changed that in the latest commit (49b13fb). Also I have created a new constant MAN_SECTIONS
and changed the loops to iterate over MAN_SECTIONS
instead of the more complicated way with range(1, 10)
.
- Add a new MAN_SECTIONS constant. - Loop over MAN_SECTIONS instead of range(1, 10) when iterating man sections. - _dfs_package_resources and get_resources now return a nested list for apps and man pages together.
This fixes something which might be a small bug in the original code, but also affecting the man pages exposition. Post-installation, pipx prints "These apps are now globally available", followed by a list of app names. However, if all of the apps are unavailable (`exposed_binary_names` is empty and `unavailable_binary_names` is non-empty), "These apps are now globally available" is not printed, when it probably should. In this case, the list of unavailable apps is printed regardless and appears somewhat out of context. The same issue affects printing of exposed and unavailable man pages.
When symbolic links are not available (e.g. on Windows), `get_exposed_man_paths_for_package` failed to produce results because it called `get_exposed_paths_for_package` with an incorrect list of man pages. Items in this list errornously included the `man<i>` path prefix.
This looks good to me. Could you fix all the tests and mark this as ready for review? |
This is to satisfy the linter. The new functions are: - get_apps_from_entry_points - get_resources_from_dist_files - get_resources_from_inst_files get_resources simply collectes the results from the above functions. The functionality is unchanged.
tests-3.11 passes now, but |
The man pages code used the str.removeprefix method which does not exist in older Python versions.
can_symlink could prevously be called without the directory existing yet, resulting in an error. Also the symlink check was performed on the "man" directory instead of individually on the "man/man<i>" subdirectories. _copy_package_resources is now _copy_package_resource and called per path from expose_resources_globally. _symlink_package_resources is removed, and instead _symlink_package_resource is called per path directly from expose_resources_globally. expose_resources_globally creates the destination directory before calling can_symlink, which fixes the error.
can_symlink is called in _get_venv_resource_paths on local_resource_dir which might not exist. Test first if the directory exists.
Now it passes all of the tests in the GitHub workflow. |
Co-authored-by: Tzu-ping Chung <[email protected]>
- Add new bullet points to the "How it Works" section. - Add description of how to use data_files with manual pages in setup.py to the "Developing for pipx" section. - Add description to the pipx install command help.
Looks good in general. Some tests on the main feature would be nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few wording suggestions.
Co-authored-by: chrysle <[email protected]>
Co-authored-by: chrysle <[email protected]>
What environment is this on (OS, or anything else specific)? The commands you posted work fine for me on Debian. |
@uranusjr @chrysle How do you deal with package specifications in |
It's Github codespace, and Github has set the environment variable I just made another test on macOS without setting |
We use a Github Action to generate those lists. You can trigger it from https://github.com/peterkuma/pipx/actions/workflows/create_tests_package_lists.yml and follow the instruction here: https://github.com/pypa/pipx/blob/main/testdata/tests_packages/README.md#generating-the-platform-specific-lists-from-the-master-list |
I find it easier to just use the |
2.1.10 fails under macos.
The reason for this appears to be that sympy is already installed in the default GitHub Codespaces image, i.e. the files |
The pull request is done on my part unless you have any more requests. I have added some tests for manual pages. The standard tests are passing. Some of the |
Thanks for the investigation. I didn't notice this before. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs merge conflicts fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pull request implementing issue #857 (Handling manpages). It implements a similar function as symlinking of apps in the local bin directory, but for unix manual pages.
--include-deps
.The changes can be tested with Python packages which install manual pages in
share/man/man<i>
. For example sympy or yt-dlp.docs/changelog.md
Summary of changes
This pull request modifies several commands and some other modules. It closely mirrors existing code for app symlinking but for manual pages (where it makes sense). It adds a new environment variable PIPX_MAN_DIR, in analogy to PIPX_BIN_DIR. Some functions implementing app symlinking are generalized to support manual page symlinking as well.
On Linux and macOS it exposes manual pages in
$HOME/.local/share/man/man<i>
.On Windows it exposes manual pages in
C:\Users\<user>\.local\share\man
, even though Windows does not have any manual page reader in its default installation. I am not sure if it makes sense to expose the manual pages on this OS at all.Test plan
Tested by running