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

Integrate manpage builds into PEP 517 build backend #79606

Merged
merged 7 commits into from
Feb 23, 2023

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Dec 15, 2022

SUMMARY

This patch creates a thin wrapper around the setuptools' PEP 517 build backend in-tree. It features an ability to request generating the manpage files in the process of building a source distribution. This toggle is implemented using the config_settings mechanism of PEP 517.
One must explicitly pass it a CLI option to the build front-end to trigger said behavior. Roughly, the packagers are expected to use the following call:

python -m build --config-setting=--build-manpages

This option has no effect on building wheels.

Related to: #79469 (comment)

ISSUE TYPE
  • Maintenance Pull Request
  • Packaging Pull Request
COMPONENT NAME

packaging/pep517_backend
pyproject.toml
setup.cfg

ADDITIONAL INFORMATION
$ python -m build --config-setting=--build-manpages --sdist
[...]
* Creating virtualenv isolated environment...
* Installing packages in isolated environment... (setuptools >= 39.2.0, wheel)
* Getting dependencies for sdist...
running egg_info
writing lib/ansible_core.egg-info/PKG-INFO
writing dependency_links to lib/ansible_core.egg-info/dependency_links.txt
writing entry points to lib/ansible_core.egg-info/entry_points.txt
writing requirements to lib/ansible_core.egg-info/requires.txt
writing top-level names to lib/ansible_core.egg-info/top_level.txt
reading manifest file 'lib/ansible_core.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
adding license file 'COPYING'
writing manifest file 'lib/ansible_core.egg-info/SOURCES.txt'
* Installing packages in isolated environment... (docutils, jinja2, pyyaml, straight.plugin)
* Building sdist...
running sdist
running egg_info
[...]
Writing ansible-core-2.15.0.dev0/setup.cfg
Creating tar archive
removing 'ansible-core-2.15.0.dev0' (and everything under it)
Successfully built ansible-core-2.15.0.dev0.tar.gz

@webknjaz webknjaz requested a review from sivel December 15, 2022 15:18
@webknjaz webknjaz self-assigned this Dec 15, 2022
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Dec 15, 2022
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 23, 2022
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jan 3, 2023
@webknjaz webknjaz force-pushed the packaging/pep517-in-tree-backend branch from d6f8b5d to 195e874 Compare January 19, 2023 00:02
@webknjaz webknjaz marked this pull request as ready for review January 19, 2023 00:05
@webknjaz webknjaz changed the title Integrate manpage builds into PEP517 build backend Integrate manpage builds into PEP 517 build backend Jan 19, 2023
@ansibot ansibot removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 19, 2023
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 19, 2023
Copy link
Contributor

@Spredzy Spredzy left a comment

Choose a reason for hiding this comment

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

I am not knowledgeable enough to +1 the code implementation, but the idea behind it. Anything that can be baked in the code directly, rather than needing documentation is a big +1

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jan 19, 2023
@webknjaz webknjaz force-pushed the packaging/pep517-in-tree-backend branch 2 times, most recently from e604bf8 to c13ce75 Compare January 19, 2023 17:37
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 19, 2023
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

I'm not opposed to this in principle, but it does feel like a slippery slope in some ways- back to the bad old days of custom DistUtils commands that we're still cleaning up in other projects 😆 . But there's definitely some benefit in being able to have declarative (and dynamic) build deps that we can test, so I'm like +0.75 for the overall thing if we can make it a little more robust.

packaging/pep517_backend/_backend.py Show resolved Hide resolved
packaging/pep517_backend/_backend.py Show resolved Hide resolved
packaging/pep517_backend/_backend.py Show resolved Hide resolved
packaging/pep517_backend/_backend.py Show resolved Hide resolved
rst_in.unlink()

rst2man_cmd = (
sys.executable,
Copy link
Member

Choose a reason for hiding this comment

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

Why not just do this inline in our interpreter with docutils.core.publish_file (or one of the other ones that takes string input, since we've already read the source in to hack on it anyway)? Locating the wrapper script that basically does the same thing feels brittle...

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I was converting a series of calls across makefiles and wasn't implementing this from scratch. So I never checked what APIs are available. FWIW, I was trying to scope this effort to reproduce what makefiles do w/o changes, so it's easier to verify that it's doing the same thing. Optimization/refactoring could be a separate PR IMO.

Copy link
Member

@nitzmahone nitzmahone Jan 19, 2023

Choose a reason for hiding this comment

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

This appears to do the same thing sans subprocess, script and intermediate file:

    from docutils.core import publish_file
    from docutils.writers import manpage
    from io import StringIO

    templated_rst_doc = rst_in.read_text().replace('%VERSION%', version_number)
    rst_in.unlink()

    publish_file(source=StringIO(templated_rst_doc), destination_path=rst_in.with_suffix('').with_suffix(''), writer=manpage.Writer())

Copy link
Member Author

Choose a reason for hiding this comment

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

@nitzmahone still, I wouldn't want to couple refactoring of the manpage builds with the packaging aspects. How do you feel about having this in a separate PR?

P.S. Pro tip: StringIO/BytesIO should be used as context managers, otherwise, they need to be closed explicitly to clean up the resources correctly. This is not always a problem, but is a nice principle to follow for consistency with other I/O interactions.

Copy link
Member

Choose a reason for hiding this comment

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

I may be mostly projecting my own behavior here 😉, but "I'll fix it later" will probably be written on my tombstone. I can live with it "as-is", but would definitely like to see it simplified with direct calls and fewer/no writes into the source tree at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

For me, it may end up being a never-ending PR refactoring that needs more and more testing, though. It'll end up blocked for longer this way.

def _make_in_tree_ansible_importable():
"""Add the library directory to module lookup paths."""
lib_path = str(Path.cwd() / 'lib/')
os.environ['PYTHONPATH'] = lib_path # NOTE: for subprocesses
Copy link
Member

Choose a reason for hiding this comment

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

This probably wouldn't be necessary if the docutils publish was called directly instead of as a subprocess (see below)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we agree to move this to a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with making this change in another PR.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 19, 2023
@webknjaz webknjaz force-pushed the packaging/pep517-in-tree-backend branch from a83a258 to a12ac37 Compare February 22, 2023 23:12
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Feb 22, 2023
@webknjaz
Copy link
Member Author

I rewrote the smoke testing in Python and tightened the pins. With this, It think that they main blocking concerns are addressed now. I'd like to keep things that are out of the scope separate.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 22, 2023
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 23, 2023
@webknjaz webknjaz force-pushed the packaging/pep517-in-tree-backend branch from ffc6434 to 6747d70 Compare February 23, 2023 13:26
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 23, 2023
@webknjaz webknjaz force-pushed the packaging/pep517-in-tree-backend branch 2 times, most recently from 4619aea to 9cd3989 Compare February 23, 2023 13:43
@webknjaz
Copy link
Member Author

@mattclay I think this is ready for the next round of reviews.

webknjaz and others added 6 commits February 23, 2023 19:34
This patch creates a thin wrapper around the `setuptools`' PEP 517
build backend in-tree. It features an ability to request generating
the manpage files in the process of building a source distribution.
This toggle is implemented using the `config_settings` mechanism of
PEP 517.
One must explicitly pass it a CLI option to the build front-end to
trigger said behavior. Roughly, the packagers are expected to use the
following call:

    python -m build --config-setting=--build-manpages

This option has no effect on building wheels.

Co-authored-by: Matt Clay <matt@mystile.com>
This test runs building and re-building sdists and wheels with and
without the `--build-manpages` config setting under the
oldest-supported and new `setuptools` pinned.

It is intended to preserve the interoperability of the packaging setup
across Python runtimes.

Co-authored-by: Matt Clay <matt@mystile.com>
@webknjaz webknjaz force-pushed the packaging/pep517-in-tree-backend branch from 9cd3989 to 38c4d22 Compare February 23, 2023 18:34
@webknjaz webknjaz requested a review from a team February 23, 2023 18:35
Co-authored-by: Matt Clay <matt@mystile.com>
@webknjaz webknjaz merged commit 5603601 into ansible:devel Feb 23, 2023
@ansible ansible locked and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.15 bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants