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.

Sign up for GitHub

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

Jump to bottom

Improve python cmake #4574

Merged
merged 4 commits into from
Oct 18, 2020
Merged

Improve python cmake #4574

merged 4 commits into from
Oct 18, 2020

Conversation

FranciscoPombal
Copy link
Contributor

@FranciscoPombal FranciscoPombal commented Apr 23, 2020 •
edited
Loading

Basically changes to use the new FindPython3: https://cmake.org/cmake/help/v3.17/module/FindPython3.html. Also drops EOL python versions.

Warning: slightly outdated info below. See #4574 (comment) for updated info.

The first commit introduces changes that make it work the same as before (on Ubuntu 18.04, I have to specify -Dboost-python-module-name=python36).

The second commit makes further use of the new variables. One benefit is eliminating the build-time dependency on python3-dev and python3-setuptools packages. It requires the latest CMake version as of writing, but I don't think that's an issue, given how easy it is to install the latest version on basically any system. But I haven't managed to get the same results as before - the site-packages directory does not seems to respect the CMAKE_INSTALL_PREFIX.

Without this commit, the discovered site-packages directory is relative, while with it, it becomes absolute. Here are the differences in the configure output as well as the install_manifest.txt, for a run with the install prefix set to /usr/local:

  • In the configure step:

Python site packages: lib/python3/dist-packages vs Python site packages: /usr/lib/python3/dist-packages

  • Diff of the install manifests:
--- install_manifest_old.txt
+++ install_manifest_new.txt
@@ -253,8 +253,8 @@
 /usr/local/lib/cmake/LibtorrentRasterbar/LibtorrentRasterbarConfig.cmake
 /usr/local/lib/cmake/LibtorrentRasterbar/LibtorrentRasterbarConfigVersion.cmake
 /usr/local/share/cmake/Modules/FindLibtorrentRasterbar.cmake
-/usr/local/lib/python3/dist-packages/libtorrent.cpython-38-x86_64-linux-gnu.so
-/usr/local/lib/python3/dist-packages/libtorrent.egg-info/SOURCES.txt
-/usr/local/lib/python3/dist-packages/libtorrent.egg-info/top_level.txt
-/usr/local/lib/python3/dist-packages/libtorrent.egg-info/PKG-INFO
-/usr/local/lib/python3/dist-packages/libtorrent.egg-info/dependency_links.txt
\ No newline at end of file
+/usr/lib/python3/dist-packages/libtorrent.cpython-38-x86_64-linux-gnu.so
+/usr/lib/python3/dist-packages/libtorrent.egg-info/SOURCES.txt
+/usr/lib/python3/dist-packages/libtorrent.egg-info/top_level.txt
+/usr/lib/python3/dist-packages/libtorrent.egg-info/PKG-INFO
+/usr/lib/python3/dist-packages/libtorrent.egg-info/dependency_links.txt
\ No newline at end of file

Also, is it a problem if the boost_python module version is different from the python library version? e.g. libboost_python36.so with libpython3.8.so? I have found that this build script allows that, with or without this PR.

Finally, should Python3_USE_STATIC_LIBS=ON depend directly on the value of BUILD_SHARED_LIBS, or does it make sense to leave them independently configurable?
@zeule any tips?

@FranciscoPombal FranciscoPombal marked this pull request as draft April 23, 2020 14:03
@zeule
Copy link
Contributor

zeule commented Apr 23, 2020

What's the Python 3 status on MacOS?

@FranciscoPombal
Copy link
Contributor Author

FranciscoPombal commented Apr 23, 2020 •
edited
Loading

@zeule AFAIK macOS does not come with any 3rd party language runtime installed by default other than Python 2, which is also deprecated there and supposed to be for "Apple use only" until it is fully removed. So users are supposed to install Python (3) manually, using the official installers or brew or something.

zeule
zeule reviewed Apr 23, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
bindings/python/CMakeLists.txt Show resolved Hide resolved
bindings/python/CMakeLists.txt Outdated Show resolved Hide resolved
@arvidn
Copy link
Owner

arvidn commented Apr 23, 2020

cmake version: https://travis-ci.org/github/arvidn/libtorrent/jobs/678690372#L787

@FranciscoPombal
Copy link
Contributor Author

@arvidn
Seems fairly easy to support the latest version, thanks to the APT repo:
https://travis-ci.com/github/Farwaykorse/test_Travis_CMake/jobs/213053641/config

@FranciscoPombal FranciscoPombal mentioned this pull request Apr 27, 2020
Closed
@Kolcha
Copy link
Contributor

Kolcha commented May 2, 2020

What's the Python 3 status on MacOS?

macOS 10.15 finally comes with Python 3, it has Python 3.7, but Python 2.7 is also available

@stale
Copy link

stale bot commented Aug 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 1, 2020
@FranciscoPombal
Copy link
Contributor Author

bump, will deal with this later.

@stale stale bot removed the stale label Aug 3, 2020
This was referenced Oct 6, 2020
Open
Merged
@FranciscoPombal FranciscoPombal marked this pull request as ready for review October 15, 2020 17:30
@FranciscoPombal
Copy link
Contributor Author

@arvidn

OK, I'm finally happy with this. There should be no difference in behavior from the current implementation, except for the fact that the egg info is no longer generated by default.

There are now two additional features related to the python bindings:

  • generate egg info and install it (OFF by default)
  • install to system site packages instead of the prefix, (OFF by default, which preserves current behavior)

Let me know if you want me to squash/reword/reorder the commits. Once this is in, I'll continue with #5197

@arvidn
Copy link
Owner

arvidn commented Oct 15, 2020

this seems like a problem: https://travis-ci.com/github/arvidn/libtorrent/jobs/400301284#L1227

@FranciscoPombal
Copy link
Contributor Author

@arvidn

Ugh. Alright, I'll try to either install a newer CMake version on Travis or just rebase #5197 on top of this in a test branch and see if it passes.

@FranciscoPombal
Copy link
Contributor Author

@arvidn

Ugh. Alright, I'll try to either install a newer CMake version on Travis or just rebase #5197 on top of this in a test branch and see if it passes.

Bad news: https://github.com/FranciscoPombal/libtorrent/runs/1261779636?check_suite_focus=true

Might be a port bug in vcpkg... I have to try without it.

zeule
zeule reviewed Oct 16, 2020
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
zeule
zeule reviewed Oct 16, 2020
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
@FranciscoPombal
Copy link
Contributor Author

FranciscoPombal commented Oct 17, 2020 •
edited
Loading

@arvidn

Alright, CMake CI passes now. Here is a run of #5197 on top of this: https://github.com/FranciscoPombal/libtorrent/runs/1266542035?check_suite_focus=true - it also works, except for debug builds and static linking, which is its own problem that I will look into in #5197.

@FranciscoPombal
Copy link
Contributor Author

@arvidn ping, the remaining CI failure seems unrelated: https://travis-ci.com/github/arvidn/libtorrent/jobs/401080845 (or if it is related, I don't know what it's about).

@arvidn arvidn merged commit 3d48e7d into arvidn:RC_1_2 Oct 18, 2020
@FranciscoPombal FranciscoPombal deleted the improve_python_cmake branch October 18, 2020 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NotTsunami NotTsunami NotTsunami left review comments

@zeule zeule zeule left review comments

@arvidn arvidn arvidn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@FranciscoPombal @zeule @arvidn @Kolcha @NotTsunami