Improved Guessing of boost_python version - #16027
Conversation
71bd861 to
4450b5a
Compare
|
This is an alternative to #15982 |
dcbaker
left a comment
There was a problem hiding this comment.
Even if the default guessing was ill advised, it works fine in configurations that only have one system python and one boost. Removing the feature will make builds that used to work fine stop working, and that is a heavy hammer.
@eli-schwartz @thesamesam are probably interested in looking at this as well.
| if mod_name in self.boost_python_libs: | ||
| mlog.error( | ||
| "Support for ", mlog.bold(mod_name), ' without specifying the version was dropped in Meson 1.12. ', | ||
| "For the correct solution for these libraries, please see ", | ||
| "https://mesonbuild.com/Dependencies.html#boost-python-and-boost-numpy" | ||
| ) | ||
| return False |
There was a problem hiding this comment.
This would be a hard compatibility break. We generally do not do that unless the feature being removed wouldn't have worked anyway ever (say because it would raise a python exception).
In this case I think we need to keep this, with a FeatureDeprecated for plain python and plain numpy. I would also not object to a mlog.warning() here that is non_fatal so that users of new Meson get the notice even if their minimum version is less than 1.12.0
There was a problem hiding this comment.
The simple way would be to check if the dependency name has any version listed and if not, print a warning and fall back to the old code.
There was a problem hiding this comment.
An even simpler version: if the version number is not specified, print the error and append the Python version of the current interpreter. It should be "the right thing" to do most of the time.
There was a problem hiding this comment.
I changed it to print a warning if any version guessing is taking place. Then if more than one possible version exists, it selects the current interpreter, if that isn't present it takes the highest version
|
I agree with the PR that we should make people switch to specifying the module name correctly, and I agree with @dcbaker that for backwards compatibility reasons we should continue to accept the old form with a warning. I think we can still raise a hard error if there are multiple libboost_python* installed. The error message in that case would say "ambiguous module, we cannot possibly know what you actually want, but hey good news it's deprecated anyway so you should go fix that". |
|
I'm putting this back on 1.12 since It's the last thing we're waiting for. @kjmeagher do you have time to work on this? If not I'll make the fixups I suggested so we can merge this, as it's the last thing we're waiting on for the 1.12 release. |
|
I tried to come up with a solution that did the correct thing and was backwards compatible with the old behavior, but this was more challenging than I anticipated. The way |
|
Okay, I'll attempt to update this tomorrow (I have other things that are higher priority for today). |
The most important thing about linking with boost python is to make sure that you match the version of python you are compiling with to the boost_python library. It is only nescessary to match to minor version not the patch version. Originally boost_python did not contain the version in the filename and it was difficult to determine which python version a given boost_python module was supposed to be. At some point, boost renamed the library to boost_pythonXYY where X is the python major version and YY is the minor version. This made things significatnly easier as you could search for a filename with the target version of python that you wanted. Meson added support to autodetect the python version in mesonbuild#5596 and mesonbuild#6855. The way the auto detection works is very ill-advised because there is no gaurentee that the boost_python version you find will match the version of python you are compiling for. This PR rectifies this by removing the autodetection feature and adding documentation informing the user the correct way to detect boost_python. If the user attempts to use auto-detection an error message is printed directing them to the documentation. Rejected Alternatives: * Find a way to pass the python instillation object to dependency. This might look a little bit nicer but doesn't really add much over appending the string to the end of the moudle name. I don't think anyone wants to add another parameter to `dependency()` just for boost python * Add boost_python support to the python module and fail if you cant find a boost_python which matches the python version. This would be more inline with the way dependency detection works in meson, but it is more complicated than just appending strings, and in my opinion not really worth the effort * A dedicated boost_python meson module. This would search both the python space and the boost space looking for a version match. This would be the most likely to find a viable boost python, but ususally people know which version of python they want to compile so this method isn't very helpful.
4450b5a to
f6e3477
Compare
|
I had to rewrite the whole way it tries to deal with python modules. Now it tags the version of python modules in the constructor. If you specify a python module without a version it will look at all the python modules and if there is exactly one it will accept that one, then if there is one matches the current interpreter it will take that one, if that fails it will take the newest version. It will warn on any attempt to use an unversioned module |
windows can understand to `42 boost python`
|
I disabled boost_python2 testing because gentoo still has python2 but the boost emerge doesn't provide boost_python2. It doesn't worth the effort to investigate |
|
|
||
| # if one of the libs matches the current interperter got with that one | ||
| for lib in pylibs: | ||
| if lib.python_version[0] == version_info[0] and lib.python_version[1] == version_info[1]: |
There was a problem hiding this comment.
The Python interpreter executing Meson is not necessarily the one that the compilation is targeting, thus sys.version_info is not necessarily the same as the one returned by the Meson's python module.
Anyhow, this would be better spelled:
| if lib.python_version[0] == version_info[0] and lib.python_version[1] == version_info[1]: | |
| if lib.python_version == version_info[:2]: |
There was a problem hiding this comment.
The main point in this comment has not been addressed!
There was a problem hiding this comment.
This was based on a suggestion from @jpakkane above, ill let him weigh in if he thinks it is still a good idea
| # Python libraries are special because of the included | ||
| # minor version in the module name. | ||
| self.python_version: tuple[int, int] | None = None | ||
| for bpl in BoostDependency.boost_python_libs: | ||
| if self.mod_name.startswith(bpl): | ||
| python_version_str = self.mod_name[len(bpl):] | ||
| if not python_version_str.isdigit(): | ||
| continue | ||
| self.python_version = (int(python_version_str[:1]), int(python_version_str[1:])) |
There was a problem hiding this comment.
This looses the handling of versioned names spelled in different ways that are currently handled in fix_python_name below. If this is intentional, it need to be documented, and it needs to be checked that the "alternative" naming schemes handled in fix_python_name are not in use on any supported platform.
There was a problem hiding this comment.
The more complicated behavior was due to Gentoo and OpenSUSE renameing the libraries circa 2020-2019 because they did not understand how boost_python was supposed to work. I do not believe there is any utility in meson continuing to support this behavior. I added a short note to the code
There was a problem hiding this comment.
I don't think a comment in the code is useful for this. Anyone interested in the history of this could simply look at the commit log. The change should be reported in the commit message. Also, "circa 2020-2019" is not very informative. Which distribution releases are affected? Are any of these still supported?
There was a problem hiding this comment.
Co-authored-by: Daniele Nicolodi <daniele@grinta.net>
The most important thing about linking with boost python is to make sure
that you match the version of python you are compiling with to the
boost_python library. It is only nescessary to match to minor version
not the patch version. Originally boost_python did not contain the
version in the filename and it was difficult to determine which python
version a given boost_python module was supposed to be. At some point,
boost renamed the library to boost_pythonXYY where X is the python major
version and YY is the minor version. This made things significatnly
easier as you could search for a filename with the target version of
python that you wanted.
Meson added support to autodetect the python version in #5596 and #6855.
The way the auto detection works is very ill-advised because there is no
gaurentee that the boost_python version you find will match the version
of python you are compiling for. This PR rectifies this by removing the
autodetection feature and adding documentation informing the user the
correct way to detect boost_python. If the user attempts to use
auto-detection an error message is printed directing them to the
documentation.
Rejected Alternatives:
Find a way to pass the python instillation object to dependency. This
might look a little bit nicer but doesn't really add much over
appending the string to the end of the moudle name. I don't think
anyone wants to add another parameter to
dependency()just for boostpython
Add boost_python support to the python module and fail if you cant
find a boost_python which matches the python version. This would be
more inline with the way dependency detection works in meson, but it
is more complicated than just appending strings, and in my opinion not
really worth the effort
A dedicated boost_python meson module. This would search both the
python space and the boost space looking for a version match. This
would be the most likely to find a viable boost python, but ususally
people know which version of python they want to compile so this
method isn't very helpful.