packaging: add ANSIBLE_PEP517_BUILD_MANPAGES envvar

[gotmax23]

Summary

Add ANSIBLE_PEP517_BUILD_MANPAGES envvar to internal pyproject build backend to control whether or not to build manpages

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

packaging

ADDITIONAL INFORMATION

build does not pass config_settings to get_requires_build_sdist(), and some tooling (e.g. Fedora's pyproject-rpm-macros) doesn't support passing config_settings down to build_sdist() either.

This commit adds a fallback to $ANSIBLE_PEP517_BUILD_MANPAGES when the --build-manpages config setting isn't passed to the backend. It also removes the workaround that unconditionally enables building manpages.

/cc @webknjaz

[ansibot]

The test ansible-test sanity --test package-data [explain] failed with the error:

Command "/root/.ansible/test/venv/sanity.package-data/3.11/75a210bb/bin/python /root/ansible/test/sanity/code-smell/package-data.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 411, in <module>
    main()
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 388, in main
    sdist_path = create_sdist(tmp_dir)
                 ^^^^^^^^^^^^^^^^^^^^^
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 193, in create_sdist
    raise Exception('make snapshot failed:\n%s' % stderr)
Exception: make snapshot failed:
warning: no previously-included files found matching 'docs/docsite/rst_warnings'
warning: no previously-included files matching '*' found under directory 'docs/docsite/_build'
warning: no previously-included files matching '*.pyc' found under directory 'docs/docsite/_extensions'
warning: no previously-included files matching '*.pyo' found under directory 'docs/docsite/_extensions'
warning: no files found matching '*.ps1' under directory 'lib/ansible/modules/windows'
warning: no files found matching '*.yml' under directory 'lib/ansible/modules'
warning: no files found matching 'validate-modules' under directory 'test/lib/ansible_test/_util/controller/sanity/validate-modules'
usage: build-ansible.py [-h] [--debug]
                        {file-deprecation-tickets,release-announcement,update-intersphinx-cache}
                        ...
build-ansible.py: error: argument command: invalid choice: 'generate-man' (choose from 'file-deprecation-tickets', 'release-announcement', 'update-intersphinx-cache')
Traceback (most recent call last):
  File "/root/.ansible/test/venv/sanity.package-data/3.11/75a210bb/lib/python3.11/site-packages/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
    main()
  File "/root/.ansible/test/venv/sanity.package-data/3.11/75a210bb/lib/python3.11/site-packages/pyproject_hooks/_in_process/_in_process.py", line 335, in main
    json_out['return_val'] = hook(**hook_input['kwargs'])
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/.ansible/test/venv/sanity.package-data/3.11/75a210bb/lib/python3.11/site-packages/pyproject_hooks/_in_process/_in_process.py", line 304, in build_sdist
    return backend.build_sdist(sdist_directory, config_settings)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmpw33mux5n/packaging/pep517_backend/_backend.py", line 133, in build_sdist
    for rst_in in _generate_rst_in_templates():
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/tmpw33mux5n/packaging/pep517_backend/_backend.py", line 91, in _generate_rst_in_templates
    subprocess.check_call(tuple(map(str, generate_man_cmd)))
  File "/usr/lib/python3.11/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '('/tmp/build-env-0ro9emgg/bin/python', 'hacking/build-ansible.py', 'generate-man', '--template-file=docs/templates/man.j2', '--output-dir=docs/man/man1/', '--output-format=man', 'lib/ansible/cli/__init__.py', 'lib/ansible/cli/adhoc.py', 'lib/ansible/cli/config.py', 'lib/ansible/cli/console.py', 'lib/ansible/cli/doc.py', 'lib/ansible/cli/galaxy.py', 'lib/ansible/cli/inventory.py', 'lib/ansible/cli/playbook.py', 'lib/ansible/cli/pull.py', 'lib/ansible/cli/vault.py')' returned non-zero exit status 2.

click here for bot help

[mattclay]

If this work-around is being removed, we might as well remove the --build-manpages option entirely, since it's not functional. The package-data sanity test, as well as the canonical-pep517-self-packaging integration test will need to be updated to use the env var as well.

[gotmax23]

0c4d0fd removes the option entirely. 5e5d439 should fix the tests.

[mattclay]

If this change is merged, it should probably be backported. However, the package-data sanity test changes will need to be omitted, since in the stable branches that test still uses the Makefile targets.

[mattclay]

@gotmax23 Is this change still needed, given that Fedora isn't using the sdist, as you pointed out in #80368?

[webknjaz]

Thank you for bringing my attention to this! The migration to the standardized PEP 517 in-tree build backend is intended to bring us closer to what the wider Python community is used to, removing all the crutches and cruft we used to have. This allows providing standard interface for building our distributions, should one need to. Although, most pip users would not go through this process and would rely on the built wheels anyway.
The downstream packaging ecosystems also standardize around the same interface nowadays which allows for better interoperability and lower maintenance burden.

I also noticed that this patch will not work for you because:

1. The spec file uses pyproject_wheel which is essentially a call to pip wheel.
2. Our in-tree PEP 517 build backend only generates manpages for sdists but not for wheels. Moreover, it would be pointless to run manpage generation when building wheels since the manpages are located outside of the Python importable packages and don't make it inside the wheel files.
3. pyproject-rpm-macros do not implement building sdists — they expect building wheels right from the source distribution which you're not even using currently. OTOH, it would be correct to build from sdists which would go through the same path as pip install src-dir — building an sdist, followed by building a wheel out of that sdist and not from the source checkout.
4. You use make PYTHON=%{python3} docs on the download directory. This would've never been necessary, if you were to just use our official sdists that already ship these manpages...

That said I don't think that this approach is acceptable since it reinvents the wheel abusing the environment side-effects outside the PEP 517 standard, plus it essentially reverses the progress we've made. For example, PEP 517 does not declare any guarantees that the build front-ends should provide access to outer env vars. The only configuration interface is config_settings, under the standard.
If some downstream front-ends have bugs in this area, they should be fixed instead of trying to build layers of hacks around this.

Besides, this is not an obstacle for Fedora packaging. With, #80255 our official upstream packages don't make use of the in-tree build backend and bundle the manpages. Anybody using those sdists, will get them without needing to generate the manpage files from jinja2 templates.
This means that you can even delete related build deps like https://src.fedoraproject.org/rpms/ansible-core/blob/de2bcead78ad982ed54ac78089120b4b1e59b319/f/ansible-core.spec#_51

I see that the specfile uses Git archives for some mysterious reason: https://src.fedoraproject.org/rpms/ansible-core/blob/de2bcead78ad982ed54ac78089120b4b1e59b319/f/ansible-core.spec#_14. Instead, it's best to rely on the official upstream source distributions rather than the unstandardized source archives.
Just change it as follows:

Source: https://github.com/ansible/ansible/archive/v%{uversion}/%{name}-%{uversion}.tar.gz
Source: %{pypi_source}

And it should do the trick (along with deleting now-unnecessary BuildRequires: python%{python3_pkgversion}-straight-plugin, BuildRequires: python%{python3_pkgversion}-docutils and getting rid of make PYTHON=%{python3} docs)

If you really want to keep building from Git checkout, you can use the same trick I did here: https://github.com/ansible/ansible/pull/80357/files#diff-cb62aeaa58754404408ea0c07b30aca399edff7a282c6465a06e0ade1969ef36R14-R19 — add pypa/build to the build deps, pre-build an sdist using the proper build command and then, feed it to your macros.

Finally, you could also make a local patch file and apply it within your RPM infra. Although, it'd probably do nothing based on my observations above, you seem to have suggested this PR based on fundamentally incorrect assumptions.

P.S. It's sad that pyproject-rpm-macros doesn't seem to support passing config_settings to build backends which is why it's a good opportunity to ask them for the implementation. It shouldn't be hard to accept an extra option and pass it around. Other downsteam distributions support it, for example gpep517 >= 3 accepts --config-json.

[gotmax23]

@gotmax23 Is this change still needed, given that Fedora isn't using the sdist, as you pointed out in #80368?

I also noticed that this patch will not work for you because

Correct, it won't work for us. I explained this in #80368. I realized part way through writing the patch that this was the case, but I figured I'd submit the PR regardless. This solution is better than the nonfuctional --config-setting=--build-manpages IMO. Skimming the pypa/build issue, I see there's some other issues with passing config_settings to get_requires_for_build_sdist() that need to be resolved.

Our in-tree PEP 517 build backend only generates manpages for sdists but not for wheels. Moreover, it would be pointless to run manpage generation when building wheels the manpages are located outside of the Python importable packages and don't make it inside the wheel files.

I suggested building the manpages and including them in the wheel's .data directory so pip installs it to {prefix}/man/man1. This is what setuptools's data_files does. If that's undesired, then you're right; generating manpages definitely shouldn't happen when building the wheel if they're not included in the wheel.

That said I don't think that this approach is acceptable since it reinvents the wheel abusing the environment side-effects outside the PEP 517 standard, plus it essentially reverses the progress we've made

Maybe it's not part of the standard, but I don't know of any frontend/build tool that doesn't pass env vars down to the backend. Other build backends such as hatchling liberally use env vars for configuration. Meanwhile, config_settings is not supported properly in setuptools (pypa/setuptools#2491) and it isn't fully supported by all build tools.

However, we established that this doesn't impact anyone other than upstream's releases to PyPI, so I won't push too hard for this.

I see that the specfile uses Git archives for some mysterious reason:

I chose to use the Github archive, as I'd prefer to build the manpages from source rather than rely on pre-generated files. I only prefer PyPI sdists when packaging projects that use setuptools_scm, as setuptools_scm does not work well with Github archives (some of these issues have been fixed recently, so I may reconsider).

If you really want to keep building from Git checkout, you can use the same trick I did here: https://github.com/ansible/ansible/pull/80357/files#diff-cb62aeaa58754404408ea0c07b30aca399edff7a282c6465a06e0ade1969ef36R14-R19 — add pypa/build to the build deps, pre-build an sdist using the proper build command and then, feed it to your macros.

This is definitely a step back. You should not have to build an sdist and extract files from it just to render manpages. We already extract the source archive once. I respect having the backend automatically include these in sdists, but that should not be the only way to generate manpages Also, your solution doesn't work. The %pyproject_wheel macro builds a wheel in the current directory and that's it. You can't feed it an sdist. I believe this is by design.

P.S. It's sad that pyproject-rpm-macros doesn't seem to support passing config_settings to build backends which is why it's a good opportunity to ask them for the implementation

I agree. I will discuss this with the maintainer.

[mattclay]

@gotmax23 Would something like #80372 work for you?

[webknjaz]

@gotmax23 Is this change still needed, given that Fedora isn't using the sdist, as you pointed out in #80368?

I also noticed that this patch will not work for you because

Correct, it won't work for us. I explained this in #80368. I realized part way through writing the patch that this was the case, but I figured I'd submit the PR regardless. This solution is better than the nonfuctional --config-setting=--build-manpages IMO. Skimming the pypa/build issue, I see there's some other issues with passing config_settings to get_requires_for_build_sdist() that need to be resolved.

With my PyPA hat on, I'd argue that anything that tries to skip the standard and rely on undocumented side-effects is not better but worse, by definition 🤷‍♂️
The build issue needs to be fixed but it is kinda trying to accommodate for a setuptools bug. Ideally, it shouldn't but because of the backward compatibility, it still does. I was going to look into fixing it in both projects. I'm not a maintainer but I'm involved occasionally, to varying extents.
Besides, the status quo does not prevent things from working, it's just a bit annoying that it returns more build deps in some cases when it could've returned less.

Our in-tree PEP 517 build backend only generates manpages for sdists but not for wheels. Moreover, it would be pointless to run manpage generation when building wheels the manpages are located outside of the Python importable packages and don't make it inside the wheel files.

I suggested building the manpages and including them in the wheel's .data directory so pip installs it to {prefix}/man/man1. This is what setuptools's data_files does. If that's undesired, then you're right; generating manpages definitely shouldn't happen when building the wheel if they're not included in the wheel.

This feature is controversial and undesired in general. Knowing that data_files is deprecated and have been discouraged for years, I've done some googling to find pointers.

It was clearly marked as deprecated in the setuptools docs in 2021. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-data-files.

data_files and the *.data directory does not have defined semantics. It depends on the installer and has no use to the project being packaged because it's non-discoverable. Plus, in the past different installers would put these in varying places: pypa/wheel#92 (comment). The recommended alternative is package_data, which has a mechanism for loading them that is OS/installer-independent.

You also seem incorrect about the install prefix. It's been pointed out in pypa/setuptools#2648, for example. Also, this test in pip https://github.com/pypa/pip/blob/fded808c8cab13e9fef65f3ac8256ec37a27d743/tests/functional/test_install_wheel.py#L218-L245 suggests that they go somewhere closer to site-packages.

A lot of discussions over the years call this feature misleading:

• Error out if absolute paths used in data_files pypa/wheel#448 (comment)
• Wheel failed include data_file pypa/wheel#214 (comment)
• bdist_wheel creates a wheel that installs incorrectly publicsuffix (data file issue) pypa/wheel#120 (comment)

Besides, even with *.data, you'd likely have to extract the build manpages from a zipfile manually. So why not do so from a tarball which is literally made for providing the source?

That said I don't think that this approach is acceptable since it reinvents the wheel abusing the environment side-effects outside the PEP 517 standard, plus it essentially reverses the progress we've made

Maybe it's not part of the standard, but I don't know of any frontend/build tool that doesn't pass env vars down to the backend. Other build backends such as hatchling liberally use env vars for configuration. Meanwhile, config_settings is not supported properly in setuptools (pypa/setuptools#2491) and it isn't fully supported by all build tools.

The point still stands, relying on side-effects is not the best option. When the build tools fix their problems, they should implement the standard and not a set of random assumptions various packaged distributions decided to make. FWIW, env vars is a factor that have a potential to contribute to non-reproducible builds and weird side-effects. So maybe, it's worth improving the standard asking the front-ends to put protections in place even.

If you really want to keep building from Git checkout, you can use the same trick I did here: https://github.com/ansible/ansible/pull/80357/files#diff-cb62aeaa58754404408ea0c07b30aca399edff7a282c6465a06e0ade1969ef36R14-R19 — add pypa/build to the build deps, pre-build an sdist using the proper build command and then, feed it to your macros.

This is definitely a step back. You should not have to build an sdist and extract files from it just to render manpages. We already extract the source archive once. I respect having the backend automatically include these in sdists, but that should not be the only way to generate manpages Also, your solution doesn't work. The %pyproject_wheel macro builds a wheel in the current directory and that's it. You can't feed it an sdist. I believe this is by design.

You can easily replace %pyproject_wheel and make doc with python -m build --no-isolation and it'll build both sdist and wheel with a single correct command (out of that sdist, as intended!). This won't amount in much overhead and would align with the official build process, making sure that there's no surprising deviations.

I disagree that it's a step back, not for us. Using internal build helpers worked in non-official environments by accident. I don't think it was ever documented to be guaranteed. We used to have different downstream packaging definitions right in this repository but got rid of them at some point. I've checked how different downstreams package ansible-core and none seem to have a problem relying on the official source distribution, instead of the source archive. We don't even test the usability of said Git archives — in fact, we've been saying people not to use them for as long as I can remember (I personally wish, we'd switched to setuptools-scm even; but because of certain limitations said Git archives did not always work as intended).

This means, that we'd be forced to maintain an extra way of doing the same thing, special-casing patches for Fedora upstream basically. It sounds like shifting the maintenance burden back to upstream. I also imagine that Fedora wouldn't notify us when it's possible to remove the hacks when they stop using them and migrate to using the proper build flow, right? So it may hang in here forever...

[mattclay]

Closing per #80368 (comment)