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

Make meson-python usage example more idiomatic #367

Merged
merged 8 commits into from
Feb 1, 2024

Conversation

rgommers
Copy link
Contributor

@rgommers rgommers commented Feb 1, 2024

  • Use dependency('pybind11'), which is all that is needed since Meson 1.1.0,
  • Don't define an unused py_mod Python module,
  • Remove custom C++ compile flags that are not present in the scikit-build-core and Maturin examples (nor seem needed),
  • Make indentation consistent,
  • Use py.install_sources instead of a more complicated install_subdir call,

- Use `dependency('pybind11')`, which is all that is needed since Meson
  1.1.0,
- Remove custom C++ compile flags that are not present in the
  scikit-build-core and Maturin examples (nor seem needed),
- Make indentation consistent,
- Use `py.install_sources` instead of a more complicated
  `install_subdir` call,
@henryiii
Copy link
Collaborator

henryiii commented Feb 1, 2024

Thanks! The example here is synced with the cookiecutter, so it needs to be changed there. I can move your changes there.

@rgommers
Copy link
Contributor Author

rgommers commented Feb 1, 2024

Ah, thank you! I was also looking for the sources to be built, but I don't think they exist, right? Rather, it's a super simple hypothetical example? If there's an actual src/ tree somewhere, I missed it.

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii force-pushed the meson-python-example branch from bb4e957 to af2e8fb Compare February 1, 2024 14:48
@henryiii
Copy link
Collaborator

henryiii commented Feb 1, 2024

The source is in {{cookiecutter.project_name}}/{% if cookiecutter.backend=='mesonpy' %}meson.build{% endif %}. We use cog to render the template (https://github.com/scientific-python/cookie/blob/main/helpers/cog_helpers.py, etc, are used to read in and render the examples as long as they are a full file example (I don't do this for partial file examples yet).

So those files are exactly what you get if you use cookiecutter/cruft/copier to render the template, are tested to work, and stay updated, etc.

@henryiii
Copy link
Collaborator

henryiii commented Feb 1, 2024

Hmmm,

../meson.build:22:3: ERROR: File src/__init__.py does not exist.

@henryiii
Copy link
Collaborator

henryiii commented Feb 1, 2024

Ahh, that one needs the package name, of course. The reason I went with install_subdir is it matched the other examples and installed the entire directory, rather than requiring (surprising?) the author to continuously update build.meson every time they add a file. While Meson recommends files listed explicitly, it's surprising to someone coming from Python where none of the other backends expect you to list every Python file.

@rgommers
Copy link
Contributor Author

rgommers commented Feb 1, 2024

While Meson recommends files listed explicitly, it's surprising to someone coming from Python where none of the other backends expect you to list every Python file.

Sure, it's a toss-up. I think CMake also recommends against globbing? Happy to go back to install_subdir here if you prefer. The strip_directory part should go though - I'd write it as:

install_subdir('src/package', py.get_install_dir() / 'package')

py.install_sources(
[
'src/{{ cookiecutter.__project_slug }}/__init__.py',
'src/{{ cookiecutter.__project_slug }}/_main.py',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that since I didn't find the source files, I completely made up _main.py - it likely doesn't exist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and py.typed is missing. Let's do install_subdir. Someone can easily move to listing things if they prefer, but the template will work better this way.

CMake does discourage globbing (less-so now that CONFIGURE_DEPENDS exists), but we don't use globbing or even CMake at all for the Python files, those are picked up and installed by the scikit-build-core following the mechanism used in other Python backends, like hatchling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how that works exactly, but in meson-python it would be a little tricky to get that right. Installing all .py files is wrong, and the .gitattributes method doesn't quite work either because that would also exclude .py files from the sdist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We rip the procedure straight from hatchling. It's based on a "package" list. The default is ["<package_name>", "src/<package_name>", "python/<package_name>"] (last of which is unique to scikit-build-core). A user can specify their own custom list, including an empty list, which disables the process. Anything in these packages is included entirely (there is a wheel.exclude option too for filtering). Anything more exotic than packages needs to be copied around via CMake and won't work as well with editable installs (hatchling has a powerful force-include dict that we might also add a some point).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context! I'll have to look into that, it does sound potentially useful.

@henryiii
Copy link
Collaborator

henryiii commented Feb 1, 2024

The strip_directory part should go though - I'd write it as:

That is invalid, the following produces an error saying only one argument is supported:

install_subdir('src/package', py.get_install_dir() / 'package')
../meson.build:22:0: ERROR: install_subdir takes exactly 1 arguments, but got 2.

This doesn't install packages correctly, it adds the "src" to the package directory:

install_subdir('src/package', install_dir: py.get_install_dir() / 'package')
E       AttributeError: module 'cookie_mesonpy' has no attribute '__version__'

It needs the directory strip to get rid of the leading "src".

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii merged commit 867da15 into scientific-python:main Feb 1, 2024
15 checks passed
@rgommers rgommers deleted the meson-python-example branch February 1, 2024 16:25
@rgommers
Copy link
Contributor Author

rgommers commented Feb 1, 2024

It needs the directory strip to get rid of the leading "src".

Ugh, you're right. I never work on packages with the src/ layout normally. Guess in a real package the meson.build file would not be at the top level, but inside src/.

@rgommers
Copy link
Contributor Author

rgommers commented Feb 1, 2024

Thanks for the quick merge!

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

Successfully merging this pull request may close these issues.

2 participants