-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Restore man page rendering #202
Conversation
--attribute="manual_title=ansi2html Manual" \ | ||
--attribute="manual_version=$(python3 -m setuptools_scm)" \ | ||
--format=manpage -D man \ | ||
man/ansi2html.1.txt' |
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.
Include this as part of normal testing in [testenv]
section to see what happens on macos. Based on the couple of hours I spent trying to get a2x to work on macos, I realised that is kinda broken and that nobody will bother to add required fixes.
As that man page is minimalistic we should find a solution to build it that can run on all platforms supported by ansi2html tool.
PS. Installing docbook on macos in to the problem, it does install just fine with brew. The problem is that the build fails somewhere with xml where it tries to access the network for a DTD but docbook is configured to run the command with network access disabled, than apparently that was hardcoded. On linux it appears to have a local database which allows it to run on offline mode, something that is not part of the homebrew install. It looked as a total mess, and I am sure that we can find a better maintained replacement.
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.
@ssbarnea what is the error output of a2x?
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.
I am not keen to let this go in, here is the proof that it breaks at least tox -e pkg
on macos:
$ tox -e pkg
pkg: recreate env because python changed version_info=[3, 11, 0, 'final', 0]->[3, 12, 0, 'final', 0] | executable='/Users/ssbarnea/.pyenv/versions/3.11-dev/bin/python3.11'->'/Users/ssbarnea/.asdf/installs/python/3.12.0/bin/python3.12' | virtualenv version='20.17.1'->'20.24.6'
pkg: remove tox env folder /Users/ssbarnea/c/os/ansi2html/.tox/pkg
pkg: install_deps> python -I -m pip install 'asciidoc>=10.1.4' 'build>=0.7.0' 'collective.checkdocs>=0.2' 'pip>=20.2.2' 'setuptools_scm>=6.0.1' 'toml>=0.10.1' 'twine>=3.2.0'
pkg: commands[0]> rm -rfv /Users/ssbarnea/c/os/ansi2html/dist/
/Users/ssbarnea/c/os/ansi2html/dist//ansi2html-1.9.0rc2.dev3-py3-none-any.whl
/Users/ssbarnea/c/os/ansi2html/dist//ansi2html-1.9.0rc2.dev3.tar.gz
/Users/ssbarnea/c/os/ansi2html/dist/
pkg: commands[1]> sh -c 'a2x --conf-file=man/asciidoc.conf --attribute="manual_package=ansi2html" --attribute="manual_title=ansi2html Manual" --attribute="manual_version=$(python3 -m setuptools_scm)" --format=manpage -D man man/ansi2html.1.txt'
<unknown>:1: SyntaxWarning: invalid escape sequence '\S'
<unknown>:1: SyntaxWarning: invalid escape sequence '\S'
a2x: ERROR: "xmllint" --nonet --noout --valid "/Users/ssbarnea/c/os/ansi2html/man/ansi2html.1.xml" returned non-zero exit status 4
pkg: exit 1 (0.51 seconds) /Users/ssbarnea/c/os/ansi2html> sh -c 'a2x --conf-file=man/asciidoc.conf --attribute="manual_package=ansi2html" --attribute="manual_title=ansi2html Manual" --attribute="manual_version=$(python3 -m setuptools_scm)" --format=manpage -D man man/ansi2html.1.txt' pid=76594
pkg: FAIL code 1 (7.07=setup[6.54]+cmd[0.02,0.51] seconds)
evaluation failed :( (7.15 seconds)
tox -e pkg 4.28s user 1.45s system 76% cpu 7.515 total
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 adds too many non-python dependencies for even building the package. That is not cool or portable.
Officially there is no way to install man pages when installing python packages so I am inclined to say that maybe the man page should not be part of the wheel distribution.
This is not the first project where I encounter this issue. "man pages not really compatible with pip". Still, I do not know what would be the optimal way to deal with this conundrum.
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.
@ssbarnea all of these dependencies are portable and can be installed via homebrew, correct? Also: Is there need for tox -e pkg
to work on macOS if we only ever do releases from Linux? The end user does not even touch tox, only contributors do, and access to Linux is easy on macOS using Docker. The alternative to the current approach is that (a) all distros have to build the man page at packaging time and install those build dependencies that they do not need right now or (b) we keep a copy of that file in Git and add CI that goes red whenever they go out of sync by rendering the man page and comparing it to the version in Git. But we're fixing a regression here. Let's first fix the regression and then discuss alternatives, things are in an unreleasable state for too long already, and improvement and fixing should not depend on each other. Thanks.
@hartwork Please review #206 -- which brings back the man page generation. Let me know if it works fine or we need to tune it a little bit more. What is annoying is that we do not have any test that ensures that the manpage is present and that can be installed as python packaging does not allow installation of manpages (due to being outside the package). |
@ssbarnea man page installation is possible from Python packages, e.g. see https://github.com/hartwork/git-delete-merged-branches/blob/070cb0783694d409d6a08a266001c3ad444395d5/setup.py#L59-L62 for a real-life example. |
@hartwork Let me rephrase: using the deprecated |
I cannot confirm that pip would leave behind man pages on removal, see: # cd "$(mktemp -d)"
# virtualenv --python=python3.11 venv
# source venv/bin/activate
# pip install git-delete-merged-branches
# find -name \*.1 | tee /dev/stderr | wc -l
./venv/share/man/man1/git-dmb.1
./venv/share/man/man1/git-delete-merged-branches.1
2
# pip uninstall --yes git-delete-merged-branches
# find -name \*.1 | tee /dev/stderr | wc -l
0 |
Re-opening while #206 is not ready… |
801c7a9
to
9d9fd46
Compare
@hartwork Please rebase. |
9d9fd46
to
11b2ebc
Compare
11b2ebc
to
376e49a
Compare
Revert parts of commit 98ce41f
376e49a
to
0fa9a0a
Compare
My arguments at https://github.com/pycontribs/ansi2html/pull/202/files#r1422642651 are being ignored, so I'll be closing this as not going anywhere… |
Fixes #193