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

We are not copying the files from the Meson install directory #344

Closed
FFY00 opened this issue Mar 14, 2023 · 16 comments
Closed

We are not copying the files from the Meson install directory #344

FFY00 opened this issue Mar 14, 2023 · 16 comments
Labels
bug Something isn't working
Milestone

Comments

@FFY00
Copy link
Member

FFY00 commented Mar 14, 2023

While reviewing #340, I found that we are not actually using the Meson install directory to copy files to the wheel, and instead we are copying them from the build directory.

This has two problems I can identify:

  • The files get installed with a wrong RPATH
    • This is because Meson sets the RPATH so that you can run stuff from the build directory
  • Exclusion parameters to install_subdir are ignored
    • This is because Meson does not export them in the metadata, so we just copy the whole directory, but we can only rely on copying from the install location, as that's the only place which will have the correct tree installed

Reproducible with all examples from the install_subdir docs:

# SPDX-FileCopyrightText: 2023 The meson-python developers
#
# SPDX-License-Identifier: MIT

project('install-subdir-exclude', version: '1.0.0')

py = import('python').find_installation()
purelib = py.get_path('purelib')

install_subdir(
  'foo',
  install_dir: purelib / 'subdir1',
  strip_directory : false,
)
install_subdir(
  'foo',
  install_dir: purelib / 'subdir2',
  strip_directory : false,
)
install_subdir(
  'foo/bar',
  install_dir: purelib / 'subdir3',
  strip_directory : false,
)
install_subdir(
  'foo/bar',
  install_dir: purelib / 'subdir4',
  strip_directory : true,
)
install_subdir(
  'new_directory',
  install_dir: purelib,
)
@dnicolodi
Copy link
Member

I added the information required to handle exclude_files and exclude_directories without having to look at the installed tree to the Meson introspection data mesonbuild/meson#11432

@dnicolodi
Copy link
Member

dnicolodi commented Mar 14, 2023

py = import('python').find_installation()
purelib = py.get_path('purelib')

This is wrong. It should be py.get_install_dir(). py.get_path() returns an absolute path as returned by sysconfig.

@rgommers
Copy link
Contributor

The files get installed with a wrong RPATH

For this part it'd be great if we had a test case, that's a lot easier to talk about then discussing what may happen in the absence of it. Would you be able to construct one @FFY00?

I added the information required to handle exclude_files and exclude_directories without having to look at the installed tree to the Meson introspection data mesonbuild/meson#11432

Thanks for fixing that. This is an issue with minor impact, and it's already fixed in Meson, so I agree that it's fine to just wait for that - no need to implement a workaround for older Meson versions that we still support. I guess we do need a test case. Either way, this is gh-317 so let's keep the discussion there and keep this for the RPATH one.

@FFY00
Copy link
Member Author

FFY00 commented Mar 14, 2023

I added the information required to handle exclude_files and exclude_directories without having to look at the installed tree to the Meson introspection data mesonbuild/meson#11432

Thanks!

This is wrong. It should be py.get_install_dir(). py.get_path() returns an absolute path as returned by sysconfig.

I'd say this is a bug in Meson, there's no point in having that method if you can't use the paths for anything, and using get_path is still required on cases where you want to mix purelib and platlib.

Anyway, as a user, I'd assume that works.

@dnicolodi
Copy link
Member

dnicolodi commented Mar 14, 2023

The documentation is clear about what that function does, I don't understand why you would expect it to do something different than what it is documented to do. If you want to mix purelib and platlib you use py.get_install_dir(pure: true) or py.get_install_dir(pure: false), as documented.

Anyway, as a user, I'd assume that works.

Well, it does not, and you receive and error telling you that it doesn't, so that you can fix your code and your expectations.

@FFY00
Copy link
Member Author

FFY00 commented Mar 14, 2023

The documentation is clear about what that function does, I don't understand why you would expect it to do something different than what it is documented to do.

For users to interpret the documentation that way, they need to be aware that Meson will change the Python paths, instead of simply honoring what is returned by sysconfig.

If you want to mix purelib and platlib you use py.get_install_dir(pure: true) or py.get_install_dir(pure: false), as documented.

I missed that bit in the documentation, thank you for pointing it out.

Well, it does not, and you receive and error telling you that it doesn't, so that you can fix your code and your expectations.

As I mentioned, the documentation is not clear IMO, and it does work in certain scenarios. Since it doesn't many other scenarios, and get_install_dir does support this use-case (thanks for pointing it out), I agree with you that users should not be doing it, and we should not support it 😅

@eli-schwartz
Copy link
Member

eli-schwartz commented Mar 14, 2023

The primary distinction between py.get_path() and py.get_install_dir() is that the former lets you get at information like
"stdlib" or "scripts" or "data", while the latter gets you "the default install_dir used by the module".

Which is relevant for a couple reasons, but most blatantly if you've configured meson with -Dpython.purelibdir=/opt/myprog/python-data or whatever. I'm sure at least one proprietary project exists somewhere, somehow, that is doing this.

...

We provide get_path() for the same reason we provide get_variable(), for the sake of thoroughness in case people want to introspect python for cases that aren't builtin... I suppose it may be plausible to have get_path recognize attempts to use it to emit purelib/platlib directories and emit a warning that get_install_dir() is probably what was meant?

@FFY00
Copy link
Member Author

FFY00 commented Mar 14, 2023

But it's irrelevant by design because Meson will mangle the install paths. If you are changing the paths for install, shouldn't you also change the paths here? In which scenario would you want a path that points to a directory that is not consistent with the Python installation Meson is installing for?

Again, I don't think Meson should be changing these paths at all in the first place, and instead pick up the right Python interpreter, but if you do, shouldn't that also be reflected here?

@eli-schwartz
Copy link
Member

I truly have no idea why people might want to get the path to for example "stdlib" (which really has nothing to do with installing anything). But then again, I also have no idea under what circumstances people want get_variable(). I am sure that inventive people doing complex things will discover reasons why such information matters to them, though. I would even venture to say it's not totally inconceivable that someone will have thought of some highly unusual reason why they want to check inside the default purelib directory for something unconnected to where files are being installed to.

And the assumption behind both those functions was that they would be advanced-usage auxiliary functions, ever since the module's original implementation way back in early 2018.

@FFY00
Copy link
Member Author

FFY00 commented Mar 14, 2023

That is fair, but as-is, given Meson's handling of Python, they're mostly unusable IMO. Fixing them is not that easy anyway. Meson picking the incorrect Python interpreter, and subsequently having to mangle its paths, is what actually needs fixing 🙃

@dnicolodi
Copy link
Member

@FFY00 you have been reporting that Meson picks the wrong python interpreter in some cases multiple times now. However, I haven't been able to understand what you mean with that, and it is not my experience. Can you be more specific about the circumstances in which Meson picks the wrong python interpreter, and what you mean by wrong?

@FFY00
Copy link
Member Author

FFY00 commented Mar 14, 2023

I have explained this multiple times on the Meson bug tracker, like in mesonbuild/meson#11133 (comment), but the summary is that Meson picks the Python interpreter just like it would any other program and then tries to adapt the paths for the prefix it wants to install, which is not portable.

Installation paths differ between interpreters. You cannot reliably pick the paths from one, change the prefix, and expect them to work on a different interpreter. If you want to install a project to /usr/local, you should pick /usr/local/bin/python.

Additionally, the interpreter in the prefix you are going to install to might even be a completely different version, so you might end up building incompatible modules. This at least won't crash, because the modules will include the minor Python version in the name, but it's just another case that showcases the issue.

@rgommers
Copy link
Contributor

rgommers commented Sep 2, 2023

This is fixed after gh-467 and previous PRs, closing. The exclude_* keywords are tested now and work as advertised, and RPATH handling seems to be happy so far too.

@rgommers rgommers closed this as completed Sep 2, 2023
@dnicolodi
Copy link
Member

RPATH handling is not correct, though. It is just that our tests do not spot the problem: we leave a spurious RPATH entry pointing at the directory in which the executable, library, or Python extension are installed. #469 fixes this issue and extends the tests.

I also wonder if we need to amend the documentation to reflect the fact that we don't actually run meson install but just re-implement it. AFAICT, this affects only two things: install_rpath support, and running scripts added with meson.add_install_script(). Both things have no effect.

@rgommers
Copy link
Contributor

rgommers commented Sep 2, 2023

AFAICT, this affects only two things: install_rpath support, and running scripts added with meson.add_install_script(). Both things have no effect.

Good point - probably worth mentioning. Low prio I think, but still useful. How about we open a new doc issue to track this?

@dnicolodi
Copy link
Member

Done. See #473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants