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

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

Open
peter-urban opened this issue Apr 20, 2023 · 11 comments
Labels
documentation Improvements or additions to documentation

Comments

@peter-urban
Copy link
Contributor

peter-urban commented Apr 20, 2023

This issue is somewhat similar to #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.

@dnicolodi
Copy link
Member

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
Copy link
Member

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.

@eli-schwartz
Copy link
Member

eli-schwartz commented Apr 20, 2023

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.

@dnicolodi
Copy link
Member

@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
Copy link
Member

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.

@eli-schwartz
Copy link
Member

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

@dnicolodi dnicolodi added the documentation Improvements or additions to documentation label Apr 22, 2023
@rgommers
Copy link
Contributor

rgommers commented Sep 1, 2023

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.

@peter-urban
Copy link
Contributor Author

peter-urban commented Sep 2, 2023

@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?

@rgommers
Copy link
Contributor

rgommers commented Sep 2, 2023

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?

@dnicolodi
Copy link
Member

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?

@rgommers
Copy link
Contributor

rgommers commented Sep 2, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants