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

python module: add option to specify a python environment to install to #9788

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

eli-schwartz
Copy link
Member

The default behavior of installing relative to prefix may be unexpected, and is definitely wrong in many cases.

Give users control in order to specify that yes, they actually want to install to a venv.

This is particularly useful for projects that use meson as a build system for a python module, where all files shall be installed into the python site-packages.

@eli-schwartz
Copy link
Member Author

/cc @rgommers @tristan957 for expressing interest in the topic

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #9788 (0e60f05) into master (5d64a15) will increase coverage by 1.21%.
The diff coverage is 87.50%.

❗ Current head 0e60f05 differs from pull request most recent head 78945fb. Consider uploading reports for the commit 78945fb to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9788      +/-   ##
==========================================
+ Coverage   66.55%   67.76%   +1.21%     
==========================================
  Files         400      402       +2     
  Lines       85122    85550     +428     
  Branches    18750    18821      +71     
==========================================
+ Hits        56651    57976    +1325     
+ Misses      24075    23072    -1003     
- Partials     4396     4502     +106     
Impacted Files Coverage Δ
mesonbuild/coredata.py 83.74% <ø> (+0.02%) ⬆️
mesonbuild/modules/python.py 67.61% <85.71%> (-1.58%) ⬇️
mesonbuild/modules/__init__.py 94.05% <100.00%> (+0.18%) ⬆️
mesonbuild/scripts/vcstagger.py 87.50% <0.00%> (-4.17%) ⬇️
mesonbuild/modules/cmake.py 71.96% <0.00%> (-3.13%) ⬇️
mesonbuild/cmake/interpreter.py 83.35% <0.00%> (-1.78%) ⬇️
mesonmain.py 66.25% <0.00%> (-1.67%) ⬇️
mesonbuild/mesonmain.py 66.25% <0.00%> (-1.67%) ⬇️
mesonbuild/mesonlib/vsenv.py 61.03% <0.00%> (-1.63%) ⬇️
... and 106 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d64a15...78945fb. Read the comment docs.

@eli-schwartz
Copy link
Member Author

@jpakkane what are your thoughts on merging this for 0.61?

Although it's past rc1, I forgot to submit this first... :( and it is behavior-preserving but should make things a lot more comfortable for python modules adopting meson, e.g. script.

I believe that this change should not cause surprises or breakage as it's very narrowly scoped. But it should definitely go in an rc2 first.

Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @eli-schwartz! This works like a charm. It'd be great to get this in 61.0, because it makes it possible to install a Python package into site-packages correctly without manually setting the prefix to the active Python interpreter's lib directory.

I've tested all four options within a conda environment on Arch Linux, and it all works as expected.

It also takes care of avoiding these warnings in my development workflow:

WARNING: Python files installed by Meson might not be found by python interpreter.
 This warning can be avoided by setting "python.platlibdir" option.
WARNING: Python files installed by Meson might not be found by python interpreter.
 This warning can be avoided by setting "python.purelibdir" option.

My only code review comment (not large/blocking) is to make the description of system somewhat clearer.

Two other thoughts/questions:

What do you think about this superceding the separate purelibdir/platlibdir options? I'm not sure there's a use for them after this feature lands, and they're badly thought out and Linux-specific concepts to begin with.

Regarding the default, it's understandable (but a little unfortunate) that it has to be prefix because of backwards compat. For any Python package that's always wrong (hence auto will never select prefix). But given that most Python devs will not be using the meson CLI but pip install . or some such thing, it's hopefully not too much of a problem in practice.

It is now possible to specify `-Dpython.install_env` and choose how python modules are installed.

- `venv`: assume that a virtualenv is active and install to that
- `system`: install to the global site-packages
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be hard to understand (it could be the conda/homebrew/etc one or the actual system provided one - you mean the former here). How about something like "install to the site-packages of the active Python interpreter (if a virtualenv, this means the Python it is derived from)"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think maybe I was thinking of that when I used the word "global", which has the connotation, to me, of "non-venv" but still within the interpreter.

What about this change?

-- `system`: install to the global site-packages
+- `system`: install to the global site-packages of the selected interpreter
+  (the one that the venv module calls --system-site-packages)

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks good, it's quite clear.


*Since 0.61.0* The `python.install_env` option is used to detect the correct
installation path. Setting to `system` will avoid making the paths relative to
`prefix`. Setting to `venv` will instead use the paths for the virtualenv the
Copy link
Contributor

Choose a reason for hiding this comment

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

Also needs explanation of what system actually does do (see my comment on line 6 of python_module_env.md).

@eli-schwartz eli-schwartz force-pushed the python-installvenv branch 2 times, most recently from bad841d to 622c530 Compare January 9, 2022 01:13
@eli-schwartz
Copy link
Member Author

What do you think about this superceding the separate purelibdir/platlibdir options? I'm not sure there's a use for them after this feature lands, and they're badly thought out and Linux-specific concepts to begin with.

I'm not entirely sure what the precise use case for them would be either. Although I guess in theory they serve a similar role as --prefix and friends if you want to control install paths with absolute precision.

Regarding the default, it's understandable (but a little unfortunate) that it has to be prefix because of backwards compat. For any Python package that's always wrong (hence auto will never select prefix). But given that most Python devs will not be using the meson CLI but pip install . or some such thing, it's hopefully not too much of a problem in practice.

I'd be willing to discuss changing the default in the future. But I do think that we need the option in the end, because users may use --prefix=$HOME/.local/ to install as non-root, and this will do the correct thing (unless perhaps Debian makes a hash of dist-packages again? I'm not sure) by installing to the posix_user installation scheme in ~/.local/lib/pythonX.Y/site-packages/.

@rgommers
Copy link
Contributor

rgommers commented Jan 9, 2022

But I do think that we need the option in the end

Yes I agree that prefix has use cases. I'm still using it to install a development build in-tree right now (it escapes me for a minute why it works better than destdir, but it does at the moment).

@jpakkane
Copy link
Member

jpakkane commented Jan 9, 2022

Policy says that after rc1 only bug fixes are allowed, no new features. So sadly this will have to wait until the next release.

@eli-schwartz eli-schwartz added this to the 0.62.0 milestone Jan 11, 2022
mesonbuild/modules/python.py Show resolved Hide resolved
mesonbuild/modules/python.py Show resolved Hide resolved
docs/markdown/Builtin-options.md Outdated Show resolved Hide resolved
@xclaesse
Copy link
Member

What do you think about this superceding the separate purelibdir/platlibdir options? I'm not sure there's a use for them after this feature lands, and they're badly thought out and Linux-specific concepts to begin with.

I think they are still needed for the case you install into prefix, which is still what you want on Linux. It is not linux-only neither, as long as you know to set PYTHONPATH when running your app, it's fine to install into your app's prefix even on Windows. I personally would even recommend doing that TBH.

@rgommers
Copy link
Contributor

I think they are still needed for the case you install into prefix,

I'm talking about -Dpython.platlibdir given on the command line as an option, see https://mesonbuild.com/Builtin-options.html#module-options. That's an absolute path, typically outside the prefix. It was only added in 0.60.0 as a workaround, and really doesn't make too much sense - any use case we can think of should be superceded by this PR.

I think you are talking here about the regular purelib/platlib specified as pure: true/false in meson.build.

@xclaesse
Copy link
Member

That's an absolute path, typically outside the prefix

They could be relative to prefix too, IIRC. But seems that's not actually documented that way...

@eli-schwartz
Copy link
Member Author

install_dir can always be absolute or relative, and in the latter case it is relative to prefix. And I am pretty sure all path options other than prefix either can be relative or must be relative, and do so in relation to prefix.

As I said above,

Although I guess in theory they serve a similar role as --prefix and friends if you want to control install paths with absolute precision.

Perhaps users are baking an installer package whose launcher sets up PYTHONPATH, idk. I haven't given much thought to it yet.

@rgommers
Copy link
Contributor

Perhaps users are baking an installer package whose launcher sets up PYTHONPATH, idk. I haven't given much thought to it yet.

I use PYTHONPATH all the time, it's a very useful env var - but it has very little to do with whether the package as a whole is pure Python or not. The way Python packages work 99.x% of the time is that you have a single tree of files which get installed to site-packages/package_name/ - or some arbitrary destdir, but that is then still not dependent on whether you have an extension module inside your package or not.

Okay, since it's not directly related to this PR, I'll shut up about it now. I just know I'm going to have to talk about Meson to a ton of people over the next year, so 2 out of 3 Python specific CLI options (after this PR is merged) being irrelevant to all of those people is not too helpful.

@tristan957
Copy link
Contributor

This whole situation and the band-aids on top of it are irrelevant in my opinion because Meson shouldn't be in the business of warning people about where they are installing files. Meson going out of its way to make bad Python distributors happy is only hurting Meson. Python developers/users are well aware of PYTHONPATH and can use it appropriately when installing outside of places that the Python interpreter can't, by default, see. Is there anyone outside of Debian who has even complained about this? I still don't even understand the necessity. Is there a bug report I can read?

@eli-schwartz
Copy link
Member Author

Is there anyone outside of Debian who has even complained about this? I still don't even understand the necessity. Is there a bug report I can read?

NGL, I really am incredibly tempted to remove the warning altogether. Especially given that ever since commit 05b5a1e we should not be getting this wrong (or not any more so on Debian than any environment in which install_env=prefix is problematic). So it really feels like the warning has run its course.

@tristan957
Copy link
Contributor

We should just force Debian to keep a Meson patch to fix this. Let them live with their decisions.

@eli-schwartz
Copy link
Member Author

eli-schwartz commented Jan 25, 2022

@FRidh could confirm it, but my understanding is that NixOS also needs the status quo behavior or explicit -Dpython.install_env=prefix to return to the status quo, since they have different per-package envs and sysconfig doesn't communicate that information.

e.g. https://github.com/pradyunsg/installer/issues/98

@FRidh
Copy link

FRidh commented Jan 26, 2022

@FRidh could confirm it, but my understanding is that NixOS also needs the status quo behavior or explicit -Dpython.install_env=prefix to return to the status quo, since they have different per-package envs and sysconfig doesn't communicate that information.

That seems correct to me. sysconfig does not contain the correct prefix for us so we need to replace it with a prefix whenever we install something into a Nix store path. See e.g. the following solution https://github.com/NixOS/nixpkgs/pull/105714/files#diff-6845adc675c162d94bec555a24f0486cb2688af7fdd7f7627a25dd53a9a7a23fR75.

…ified

Needed to check exclusivity of module options.
The default behavior of installing relative to prefix may be unexpected,
and is definitely wrong in many cases.

Give users control in order to specify that yes, they actually want to
install to a venv.

This is particularly useful for projects that use meson as a build
system for a python module, where *all* files shall be installed into
the python site-packages.
@eli-schwartz
Copy link
Member Author

xclaesse gave a lgtm via IRC.

@eli-schwartz eli-schwartz merged commit 78945fb into mesonbuild:master Feb 23, 2022
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.

6 participants