mesonbuild / meson-python

Meson PEP 517 Python build backend
https://mesonbuild.com/meson-python/
MIT License
118 stars 59 forks source link

BUG: Allow installation directories #571

Closed romsom closed 4 months ago

romsom commented 4 months ago

Currently meson-python fails to execute a custom_target() which specifies a directory as output and enables installation. You need this if you have a custom_target() for which you don't know the exact set of output files in advance. When run directly (meson build, meson install -C build), meson installs the directory without any problems. Apparently (stated on the meson IRC channel) this is explicitly allowed in meson, although the documentation doesn't seem to either forbid or allow it explicitly.

A concrete example use case is when you want to generate html documentation using sphinx-build, which generates not only one html page per input, but also places necessary images, css and js files into its output directory.

The issue is in _install_path() in __init__.py which tries to write() to the output file not checking if it is a regular file first.

This PR solves the issue by checking if the current path is a directory and if so calls _install_path() recursively on all of the path's children.

dnicolodi commented 4 months ago

This should be fixed, but I don't grok the example use case: documentation does not belong into a Python wheel. Where in the wheel content do you want to install the documentation?

romsom commented 4 months ago

Yes, I tend to agree after some more thought. I used to put documentation in share/doc and share/man depending on the type, but that's definitively non-standard and very unix-y.

romsom commented 4 months ago

On that matter, but still off-topic: Do you have a pointer where I can learn more about how project documentation is usually handled with python packages?

romsom commented 4 months ago

Should I look for another example or are you fine with the abstract rationale? Also, as you can see, this is a minimal effort fix, but since I had already done it, I submitted it as a PR right away instead of opening an issue. I'd fixup the commits into one if you want to use the fix as implemented here. Let me know if you need anything for this to proceed.

dnicolodi commented 4 months ago

To consider merging this, we would also need a test, and the commits to be squashed together. However, I think that the fix is incomplete: the installation path traversal used by the custom Python module loader for editable installations expects that only install_subdir installs directories.

rgommers commented 4 months ago

I have examples of a custom_target with multiple outputs that need to be installed (e.g., here), but none with a directory. I can imagine it being useful though. For example for code generation where the outputs depend on the content of the input source file(s). We had that case with f2py, but fixed it on the f2py side by writing empty output files if needed rather than omitting them completely (so we could list output files reliably).

A test could use a codegen.py script like this:

#!/usr/bin/env python3
"""
Write out a number N of .py files; N cannot be known in advance.
Simplified with a random number generator. Real-world code may
have depend N on the content of an input file instead.
"""
import random

def write_files():
    with open('always.py', 'w') as f:
        foo = 42

    if random.randint(0, 1) == 1:
        with open('sometimes.py', 'w') as f:
            bar = 43

def main():
    parser = argparse.ArgumentParser()
    parser.add_argument("infile", type=str,
                        help="Path to the input file")
    parser.add_argument("-o", "--outdir", type=str,
                        help="Path to the output directory")

if __name__ == "__main__":
    main()

And then used in meson.build like:

custom_target('fblas_module',
  output: ,  # TODO: how does this work, do you just leave out the `output` keyword?
  input: 'an_input.py',
  command: [codegen.py, '@INPUT@', '-o', '@OUTDIR@'],
  install: true,
  install_dir: py.get_install_dir / 'mypkg',
  install_tag: 'python-runtime',
)
rgommers commented 4 months ago

This was addressed with a more targeted fix including a test case along the lines of the comment above in gh-584. So I'll close this PR. Thanks @romsom and @dnicolodi!