meson-python icon indicating copy to clipboard operation
meson-python copied to clipboard

Document meson-pythons requirements for the "install_dir" option of e.g. configure_file

Open peter-urban opened this issue 2 years ago • 11 comments

This issue is somewhat similar to https://github.com/mesonbuild/meson-python/issues/233, but different.

I had the following problem: I tried to configure an __init__.py file and install it into a subdirectory of the python install_dir using the pymod.get_install_dir function:

pymod = import('python').find_installation(pure: false)
configure_file(input : '__init__.py',
                output : '__init__.py',
                configuration : conf_data,
                install_dir : pymod.get_install_dir() + 'some_subdirectory/', 
                )

This works with meson, but from meson-python I get an error: "Could not map installation path to an equivalent wheel directory: ...". The problem seems to be the "+ 'some_subdirectory/'" because the solution is to put the subdirectory name INTO the get_install_dir command using the subdir keyword_argument:

pymod = import('python').find_installation(pure: false)
configure_file(input : '__init__.py',
                output : '__init__.py',
                configuration : conf_data,
                install_dir : pymod.get_install_dir(subdir: 'some_subdirectory'), 
                )

I think this behavior should be documented. If you tell me where to add this, I can create a pull request.

peter-urban avatar Apr 20 '23 11:04 peter-urban

The issue is that the + operator forces the generic path (I don't know what the real object name is) returned by py.get_install_dir() to be coerced into a string. Therefore, the generic path information used by meson-python to map the files into the wheel locations is lost. If you use the / path concatenation operator, everything works as expected.

I'm almost tempted to suggest that this is a bug in Meson. @eli-schwartz Do you have any opinion on this matter?

dnicolodi avatar Apr 20 '23 12:04 dnicolodi

Investigating the Meson side of things: py.get_install_dir() returns a OptionString object that inherits from the String object but holds a concrete path and a path wit placeholders for installation directories (/usr/local/lib/python3.10/site-packages/foo vs {py_purelib}/foo). meson-python needs the path with the placeholders. The OptionStrings object overrides the op_div() method for implementing the / path concatenation operator for both the concrete path and the placeholder path. However, it does not override any other method, thus any operation on an OptionString that is not path concatenation results in the OptionString being converted to a String and loosing the placeholder information. I think that overriding at least also the + operator too would make sense.

dnicolodi avatar Apr 20 '23 12:04 dnicolodi

No, the plus operator explicitly does not perform path operations. By sheer coincidence, the value of get_install_dir returns something that ends in a / directory separator, if and only if you don't pass the "subdir" kwarg to it. Because if you don't, then it tries to join the empty string and that results in a trailing slash.

Using get_option('datadir') + 'applications', for example, and expecting to get the share/applications/ directory for installing Linux .desktop files to, won't work. You'll get "shareapplications" and the install stage will produce incorrect results.

Using the string addition method in meson.build for paths is approximately as idiomatic as using the string addition method in python for paths. Things like os.path.join and pathlib.PurePath exist for a reason.

My original implementation of OptionString made the value judgment that I wasn't comfortable guaranteeing that anything other than actual path operations would be capable of returning a new string that bears a relationship with the old string -- so I intentionally refrained from overriding any method other than the path join method.

eli-schwartz avatar Apr 20 '23 13:04 eli-schwartz

@eli-schwartz After reading the original reply (from the GitHub notification email) I thought that I didn't explain well what I meant. Reading the edited version of the reply, I'm not so sure anymore this is the case. However, just in case, here is the clarification.

OptionString('{datadir}', '/usr/share') + 'foo'

currently returns

String('/usr/sharefoo')

I was wondering whether it should return

OptionString('{datadir}foo', '/usr/sharefoo')

which is still not what the user wanted, but maybe it would more consistent data model wise.

My original implementation of OptionString made the value judgment that I wasn't comfortable guaranteeing that anything other than actual path operations would be capable of returning a new string that bears a relationship with the old string -- so I intentionally refrained from overriding any method other than the path join method.

I would be comfortable with all operations on OptionString other than path join with the / operator being forbidden without an explicit conversion to a string. I don't think this compatibility breakage would be a great idea. However, raising warnings maybe is a workable compromise. I don't know if the Meson language has a way to explicitly convert something to a String object.

dnicolodi avatar Apr 20 '23 15:04 dnicolodi

There is the precedent of bool.to_string() and int.to_string(). However, from the Meson documentation it seems that the users should not be aware of OptionString objects, which should behave as str objects. I don't see enough wiggle room to implement a stricter behavior.

dnicolodi avatar Apr 20 '23 15:04 dnicolodi

Right, it exists solely for internal tracking e.g. for cases like introspection files.

eli-schwartz avatar Apr 20 '23 15:04 eli-schwartz

We don't have a single usage of py.get_install_dir in the docs. It's not recommended to use it, and is only rarely needed with custom_target's. Do we really want to document this? And if so, on what page?

I'd personally be inclined to close this issue.

rgommers avatar Sep 01 '23 19:09 rgommers

@rgommers Configuring and installing a python file into a subdirectory seemed to me like something that other people using meson-python may want to do as well. (I can be wrong of cause, and maybe this is just too niche)

However, I also cannot find a section in meson or meson-python where this issue would currently fit.

What do you think about adding a "common pitfalls" chapter under References in meson-python documentation where this (and possibly other) issues could fit?

peter-urban avatar Sep 02 '23 11:09 peter-urban

True, it's configure_file too in adding to custom_target - install_subdir too. You're right I think, probably common enough to explain.

I'm not quite sure about "common pitfalls", because such sections tend to go out of date and be a bit random. it could work though.

An alternative option is to have a howto like Controlling install locations. That, or some broader section that covers each commonly used Meson command and anything with them specific to Python packages (if it's 100% generic, it should go in the Meson docs):

  • for multiple commands: py.get_install_dir
  • run_command: should explain the use of shebangs and not using `run_command(['python', 'my_script.py']),
  • custom_target: output can not be passed to py.install_sources
  • configure_file: no other specifics I think
  • install_subdir: ?

@dnicolodi any opinion here?

rgommers avatar Sep 02 '23 11:09 rgommers

I was just writing that py.get_install_dir() should be used in a few more place, with install_subdir() probably the most popular for installing Python packages. I would cover install_subdir() in the introductory documentation, as an alternative to py.install_sources(). Otherwise a Controlling install locations section looks appropriate to me.

  • custom_target: output can not be passed to py.install_sources

Is this a limitation that needs to be solved on the meson side, or is it intended, for some reason?

dnicolodi avatar Sep 02 '23 11:09 dnicolodi

Is this a limitation that needs to be solved on the meson side, or is it intended, for some reason?

There's still something to solve I believe. Not 100% sure anymore if it's only CustomTargetIndex or both that and CustomTarget. Eventually I'll get back to it, to solve: https://github.com/scipy/scipy/blob/f3c722125f4e29004916f2c8224f6dd44609766c/scipy/linalg/meson.build#L340-L364. In SciPy we have that kind of problem in various places - headers generated from Python scripts can't be installed the way you'd expect.

rgommers avatar Sep 02 '23 12:09 rgommers