DOC: major documentation rewrite #276
Conversation
FFY00
force-pushed
the
new-docs
branch
3 times, most recently
from
df914c2
to
1302022
Compare
|
Sorry for the big PR, I'd recommend you having a look to see if there is anything major wrong. If there isn't anything major wrong, we should probably merge, and then we can send followup PRs to improve it. |
rgommers
added
documentation
There was a problem hiding this comment.
Thanks @FFY00! This looks like a good structure. I added some comments from a first read.
Content suggestions I have:
- a how-to for controlling what files get included in an sdist (with
.gitattributes) and that a change to a vcs-tracked file gets included but a new file does not. It's mentioned briefly in the section on sdist, but could use more detail and an example `.gitattributes file - a tutorial page on cross-compiling (I can fill it in, I am dealing with bits and pieces of this in multiple places already)
- a how-to page on including a VCS-generated version (e.g. with
fs.existssnippet in Support dynamic version generation, including from git tags and with git commit hashes #159 (comment)) - A Who uses meson-python page, linked from the front page.
docs/tutorials/introduction.rst
Outdated
| project('purelib-and-platlib', 'c') | ||
|
|
||
| py_mod = import('python') | ||
| py = py_mod.find_installation() |
There was a problem hiding this comment.
I suggest using find_installation(pure: false), that's more robust.
There was a problem hiding this comment.
From https://mesonbuild.com/Python-module.html#find_installation
pure: On some platforms, architecture independent files are expected to be placed in a separate directory. However, if the python sources should be installed alongside an extension module built with this module, this keyword argument can be used to override the default behavior of.install_sources(). since 0.64.0
Hummm... I do not follow. AFAICT this will just force sources to be placed in platlib.
There was a problem hiding this comment.
Which is what you want, since you have C code in your package. This is a safety measure; otherwise you have to plaster pure: false everywhere you use py.install_sources, and if you forget it just once you end up with a broken package. That's why it was added: https://mesonbuild.com/Release-notes-for-0-64-0.html#pythonfind_installation-now-accepts-pure-argument
There was a problem hiding this comment.
Why do you end up with a broken package? Is there an assumption somewhere that all the code in a package needs to be installed either in {platlib} or in {purelib}? meson-python does not do this assumption, and actually has code to handle this case. If this is the case, it would be nice to detect the circumstance and raise an error in meson-python. However, I don't see why this is the case. Also, in most platforms, {platlib} and {purelib} are the same path.
There was a problem hiding this comment.
Is there an assumption somewhere that all the code in a package needs to be installed either in
{platlib}or in{purelib}?
Yes of course. Say you have a foo.py file and bar.so extension module, and you do a relative import from .bar in foo.py, that only works if the two files are in the same directory. Which is the kind of thing you only see is broken when some niche Linux distro doesn't have purelib and platlib pointing to the same path (not Ubuntu, which is typically the one thing people test in their CI). This whole purelib/platlib concept is just awful.
meson-python does not do this assumption, and actually has code to handle this case. If this is the case, it would be nice to detect the circumstance and raise an error in meson-python.
Not every build goes only through meson-python. If it's broken for plain meson setup/install, that's still bad. Raising an error SGTM.
There was a problem hiding this comment.
Setting this on the installation object will flip the default value on the install_sources() and get_install_dir() functions. This is the correct, sensible, sane, and only reasonable thing to do if you have a top-level module with, nested inside it, a mixture of pure python and compiled extensions: it means that by default, you install pure python together with the compiled extension.
It doesn't prevent you from passing pure: true to individual calls where you do intend to install some freestanding top-level pure-python modules to purelib anyways. And I agree, that's a valid use case -- although I expect it's also incredibly rare.
Failing to place pure python files in the platlib when they are nested inside a package together with platlib extensions, means that you end up with one package being a namespace package and the other not, with corresponding... abuses... of the importlib search path. You are not going to end up happy if that happens, because at least one user is going to be on a distro which splits the two, and then disaster is going to happen and it's not going to be pretty.
We should absolutely document the default recommended approach, as the one that doesn't break people. :)
There was a problem hiding this comment.
I think it's reasonable to set that, but IMO it would be best to not set it, and add an admonition below pointing to a page that explains this in-depth.
Failing to place pure python files in the platlib when they are nested inside a package together with platlib extensions, means that you end up with one package being a namespace package and the other not, with corresponding... abuses... of the importlib search path. You are not going to end up happy if that happens, because at least one user is going to be on a distro which splits the two, and then disaster is going to happen and it's not going to be pretty.
We should try to detect these cases and warn users.
There was a problem hiding this comment.
I added this
.. admonition:: Mixing pure and native code
:class: caution
If you are mixing pure (``.py``) and native modules in your project, please
check out :ref:`reference-quirks-mixing-purelib-and-platlib`, which explains
the common foot-guns when doing so, and how to make sure you install your
modules correctly.There was a problem hiding this comment.
@FFY00, I disagree - this is not a choice between two equivalent alternatives. pure: false is so far better, it must be used. You have no idea how much time this has already cost me in subtle bugs, and how tricky it is to debug. Please just accept the comment. Keeping a bad default and then adding cognitive load for the user with a note explaining why they should not follow your example code because of footguns is not useful.
There was a problem hiding this comment.
I changed it.
IMO this is the kind of issue we can, and should be trying to detect.
It seems like a good strategy to merge when the structure is right (it matches the suggestion in gh-197; let's give everyone a couple of days to have at least a high-level look at this) and what is there in terms of text is in decent shape. So we get the improved docs for the issues that this already closes. And then we can parallelize filling in the most important blanks. |
|
By the way, on updating this PR: it would be quite useful to not rewrite history as you go. That makes it impossible to check what changed for reviewers. Rather, push new commits as updates, and then squash commits right at the end. |
Yeah, sorry. Github isn't a great git review platform. It would be awesome if they added support for rebase-merges with autosquash, that way I could push fixup commits for specific commit, and not have to squash the whole history. Currently, to update a specific commit I need to force-push, with the only alternative being committing to squash the whole history on merge. FYI you can actually click in "force-pushed" in the message to see what changed. I want to push some of your feedback as separate commits, so instead of force-pushing, I can just push the fixup commits without squashing for review, and then when it comes time to merge, I will squash them and merge. Would that work for you? The other, more annoying, alternative would be to just push those changes as a separate PR. |
|
I fixed the smaller stuff you mentioned, I will do the rest monday 👍 |
docs/who-uses-meson-python.rst
Outdated
|
|
||
| ************************** | ||
| Who uses ``meson-python``? | ||
| ************************** |
There was a problem hiding this comment.
I don't like adding a list of projects using meson-python to the documentation, or anywhere else. As we don't have any need for advertisement (and I'm pretty sure we don't want to be advertising any other project) it does not serve any purpose and it is going to be a burden to maintain. For the users, it is much better to have a short and curated list of projects that follow best practices and that can be used as examples.
Unless the listed projects are known to following best practices, the list is not going to be useful for the users. I don't think we have resources to review all the projects submitted to be listed, and to keep our review current.
I would much prefer it this would be something like:
Curated list of projects using ``meson-python``
===================================
The following projects use Meson as their build system and meson-python to produce Python wheels. They are roughly listed in order of complexity:
- `siphash24 <https://github.com/dnicolodi/siphash24>`_ A very simple project. It demonstrates how Meson makes it trivial to compile a CPython extension written in Cython via a simple tempate engine and link it to a library compiled from a Meson subproject. Also an example of how to use `cibuildwheel`_ to produce Python wheels for several platforms.
- (i don't know the peculiarities of the other projects)
- `SciPy <https://github.com/scipy/scipy>`_ Probably the most complex Python project using Meson and ``meson-python``. Staring at the build definition too long may cause headaches.
There was a problem hiding this comment.
Are the new updates good for you, or do you change the wording further?
There was a problem hiding this comment.
@dnicolodi please let me know if you'd like further changes, I think this is the only blocker for merging this.
There was a problem hiding this comment.
I'm not completely happy with the section name. I would prefer something like "Projects using meson-python".
|
The force-push was from a rebase to solve conflicts. The commits being reviewed haven't been changed other than just solving conflicts. As mentioned previously, updates to the original commits are being done via fixup commits. They will be squashed after approval, just before merging. |
|
@rgommers I think I've address all your comments, could you do a second pass? |
|
|
||
|
|
||
| To enable the verbose mode on any existing ``meson-python`` editable install, | ||
| you simply need to set the ``MESONPY_EDITABLE_VERBOSE`` environment variable |
There was a problem hiding this comment.
It is not related to this PR but it came to my attention scrolling through the added documentation: I think that the mesonpy name should not be used and that we should stick with meson-python everywhere. Thus I would prefer if we standardize on MESON_PYTHON_ as the prefix for environment variable names.
docs/who-uses-meson-python.rst
Outdated
|
|
||
| ************************** | ||
| Who uses ``meson-python``? | ||
| ************************** |
There was a problem hiding this comment.
I'm not completely happy with the section name. I would prefer something like "Projects using meson-python".
Closes mesonbuild#197, mesonbuild#230, mesonbuild#253, and mesonbuild#264 Sets us up to fix mesonbuild#138, mesonbuild#233, and mesonbuild#224 Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
This way we don't need to update both places. Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Okay, will do. |
Closes #197, closes #230, closes #253, and closes #264
Sets us up to fix later #138, #233, and #224
Signed-off-by: Filipe Laíns lains@riseup.net