Better integration of setup.py to invoke b2 #5325

AllSeeingEyeTolledEweSew · 2020-12-02T23:34:01Z

Hi,

I rewrote setup.py in the python bindings to be better-integrated with the b2 build system.

My use case is that I want to more easily build wheels of libtorrent. Specifically, I want to use tox and pre-commit in my libtorrent-based python project. These (and other tools) set up isolated environments and try to install dependencies, and since libtorrent isn't published on pypi, the easiest way to make these tools work is run a local pypi instance which provides access to libtorrent.

It was possible to make wheels before, but this integration fix makes it much easier. I now can build wheels with custom b2 compilation, like so:

$ python setup.py build_ext -j9 --debug --libtorrent-link=static bdist_wheel

I deleted all the existing build infrastructure from setup.py as I don't believe it was very useful: the --bjam mode didn't seem to actually integrate with distutils. It just invoked b2 from the global level, but didn't allow for passing options. The non---bjam mode seemed very limited and wasn't referenced at all in the build docs.

I can't find any reference that setup.py is actually used by anyone today; debian and alpine's build scripts just invoke autotools and don't use setup.py. This is appropriate for them, but setuptools is a better choice for packaging in the distro-agnostic python world.

AllSeeingEyeTolledEweSew · 2020-12-02T23:39:58Z

I'm not sure what the relationship should be of this PR to #4218; it needs some discussion with the author.

arvidn · 2020-12-03T02:30:10Z

it would be great if you could extend the GH action for building the python binding to use this, and maybe even upload the package as an artifact

AllSeeingEyeTolledEweSew · 2020-12-03T08:24:04Z

it would be great if you could extend the GH action for building the python binding to use this, and maybe even upload the package as an artifact

Interesting. I'd love to achieve the same goals as #4218, but it's a big PR, and I'd at least like to split out the artifact building part. I think it's going to get hairy.

Given that you have this:

libtorrent/bindings/python/Jamfile

Lines 137 to 140 in f726d10

    
           if <boost-link>static in $(properties) && $(BOOST_VERSION_TAG) < 1_74 && <target-os>linux in $(properties) 
        
           { 
        
           	ECHO "WARNING: you cannot link statically against boost-python on linux before version 1.74.0, because it links against pthread statically in that case, which is not allowed" ; 
        
           }

If this is true (though the boost-python docs don't mention it), then to build an appropriate linux wheel we need boost-python 1.74, which isn't in ubuntu yet, so we need a local download-and-install script as in #4218 . It just seems a bit much for this PR

arvidn · 2020-12-03T11:04:27Z

sure, I don't want to bloat this PR. But, if this code you wrote isn't running on CI, how do I know it works?

arvidn · 2020-12-04T10:05:06Z

.github/workflows/macos.yml

        run: |
          cd bindings/python
-          b2 -j2 stage_module stage_dependencies cxxstd=11


the build is failing be cause cxxstd=11 isn't passed to b2 right now it seems

oh, is cxxstd=11 required on macos right now?

setup.py could detect and pass this, but should the jamfile enforce it instead? 🤔

in later version (RC_2_0 and master) the Jamfile defaults to cxxstd=14 and cxxstd=17 respectively. I just haven't made that change to RC_1_2 because it supports being built with an old version of boost where the cxxstd option didn't exist yet

arvidn · 2020-12-04T10:06:41Z

.github/workflows/linux.yml

-        run: |
-          sudo apt install libboost-tools-dev libboost-python-dev libboost-dev libboost-system-dev
-          echo "using gcc ;" >>~/user-config.jam
-          echo "using python ;" >>~/user-config.jam


Is there a good reason not to configure boost build? It seems risky to rely on its default heuristic to pick gcc

This was intentional; the typical expectation with setuptools is that it should be able to pick sane defaults without special configuration (except for installing dependencies to build extensions).

I could add a separate test to pass toolset=gcc. I'm still trying to get a feel for what other specific feature properties to test.

I would like to preserve the test of b2 in the bindings/python directory. would you mind restoring this and adding another job that runs setup.py?

I would like to keep this python job. would you mind just reverting this file?

arvidn · 2020-12-04T10:07:22Z

.github/workflows/macos.yml

        run: |
          brew install boost-build boost boost-python3 [email protected]
-          export PYTHON_INCLUDE=`echo /usr/local/Cellar/[email protected]/*/Frameworks/Python.framework/Versions/3.9/include/python3.9` ;
-          echo "using darwin ;" >>~/user-config.jam
-          echo "using python : 3.9 : /usr/local/opt/[email protected]/bin/python3 : $(PYTHON_INCLUDE) : /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.9/lib/python3.9/config-3.9-darwin : <target-os>darwin ;" >> ~/user-config.jam


Are you confident boost build will find the correct python headers and library without configuring it here?

in fact, it looks like it's building against python 2.6 in the github action now, since you removed this configuration step

I'm confident that I want this to be the test :)

Another expectation with setuptools is that it builds the package for the running interpreter, so ideally setup.py or the jamfile should be able to work out these paths. I don't have that much experience with macos though, so I'm not sure if this expectation is reasonable.

ok, makes sense. I would also like to test that the python binding builds using b2 using the boost build user configuration.

arvidn · 2020-12-07T08:13:35Z

I think this error looks like it's caused by setuptools trying to build the extension. That's not the intention, right? it should use the binary built by b2

AllSeeingEyeTolledEweSew · 2020-12-07T08:50:17Z

I think this error looks like it's caused by setuptools trying to build the extension. That's not the intention, right? it should use the binary built by b2

Yes. Looks like I actually needed to pass the extension suffix to boost so its output is recognized by setuptools; the debian version shadowed this problem.

I'm doing most of my testing on a debian-based arch and I'm finding that debian has patched the daylights out of python.jam from boost-build, with prety significant behavior changes. It makes me think that all the CI should be run on non-ubuntu linux as well as ubuntu, or else install boost from source.

AllSeeingEyeTolledEweSew · 2020-12-07T10:35:42Z

I made setup.py default to passing cxxstd=11, only on osx. Hopefully this is appropriate.

I put setup.py invocation on osx in a separate github action, which is passing now.

It's not currently invoked on windows. Should I create a separate appveyor matrix entry to invoke setup.py, separate from one that builds the binding with b2? Appveyor already takes forever.

I doubt the linux CI failure is a result of this PR.

AllSeeingEyeTolledEweSew · 2020-12-09T18:49:30Z

I rebased to make sure the CI failure wasn't mine.

@arvidn could you advise on whether you'd like to use appveyor or github actions for new windows CI?

FranciscoPombal · 2020-12-09T19:14:52Z

@AllSeeingEyeTolledEweSew

I rebased to make sure the CI failure wasn't mine.

@arvidn could you advise on whether you'd like to use appveyor or github actions for new windows CI?

I was working on GH Actions CI for Windows + CMake here #5197, I should finish it relatively soon.

arvidn · 2020-12-10T18:46:23Z

.github/workflows/macos.yml

+          cd bindings/python
+          /usr/local/opt/[email protected]/bin/python3.9 test.py
+
+   python_b2:


it looks like you preserved the existing test here. would you mind re-ordering these jobs to make the diff smaller, and reflect the fact that the existing test is unchanged?

arvidn · 2020-12-10T18:46:58Z

lgtm, except the github workflow changes

arvidn · 2020-12-17T13:09:58Z

.github/workflows/python.yml

+    - name: install dependencies (linux)
+      if: runner.os == 'Linux'
+      run: |
+        sudo apt install libboost-tools-dev libboost-python-dev libboost-dev libboost-system-dev python3 python3-setuptools


you should probably rebase now that #5749 landed, as well as add those sudo apt update here as well

arvidn · 2020-12-17T13:12:00Z

bindings/python/setup.py

+        self.pic = None
+        self.optimization = None
+        self.hash = None
+        if platform.system() == "Darwin":


I don't think this distinction is important. You just happen to have an older compiler on MacOS that doesn't default to C++11. The more interesting question is, which ABI is python itself use? perhaps C++11 is too old.

I don't think this distinction is important. You just happen to have an older compiler on MacOS that doesn't default to C++11.

I see. Do you have opinions on whether it would be better to default toolset=gcc on darwin, rather than default cxxstd=11? I'll play with this.

I guess what's really needed is a default of cxxstd=11 if toolset=darwin, but setup.py doesn't know if toolset=darwin will be autodiscovered in b2. IMO would be much better to just apply this in some Jamfile in RC_1_2, rather than here.

The more interesting question is, which ABI is python itself use? perhaps C++11 is too old.

cpython's ABI is C only. So cxxstd's ABI just matters for the extension's downstream dynamic linkage. Ideally, when packaging a wheel for pypi, the choice of cxxstd should never matter to consumers.

(There are various C ABIs defined, but setup.py's scope is to only know about the ABI of the running interpreter, and configure boost to build for that)

IMO would be much better to just apply this in some Jamfile in RC_1_2, rather than here.

It is in RC_2_0 and later

cpython's ABI is C only. So cxxstd's ABI just matters for the extension's downstream dynamic linkage.

Which C++ standard is used affects which C standard the compiler is using as well. The C standard doesn't define an ABI, just like C++ doesn't. It may be easier for platforms to maintain a stable ABI in C, but there's no guarantee. windows/MSVC is a good example.

I imagine specifying stdcxx=11 here is safe, but with MSVC, the version of the compiler depends on the version of python.

In truth I don't think we need to work too hard to solve this here.

I don't think invoking setup.py with no args needs to produce the perfect most-compatible build. When packaging wheels for pypi, I expect we'd do things like --libtorrent-link=static --boost-link=static which wouldn't be appropriate in most other cases.

I just wanted setup.py with no args to produce something useful, both to help users, and to trim down os-specific overrides in CI. I expect the impact of changing this default later will be low.

For what it's worth, I haven't seen evidence that python is required to be compiled with any particular C standard on mac/linux.

On linux particularly, I struggle to think of any problems that would be caused for an executable linking to a library, if they differed in C standard alone. I would expect versioned symbols in libc to resolve the differences (in a perfect world). I don't have much macos experience so I don't know if they have something similar.

AllSeeingEyeTolledEweSew · 2020-12-17T23:37:30Z

I think it's ready now.

Note that I don't currently have a windows workflow to invoke setup.py, because figuring out how to install boost is daunting. Once #5197 lands, I'll reuse its setup methods to extend my python.yml.

arvidn · 2020-12-19T07:24:59Z

tools/set_version.py

        elif "version = '" in line and name.endswith('setup.py'):
-            line = "    version='%d.%d.%d',\n" % (version[0], version[1], version[2])
+            line = "    version=\"%d.%d.%d\",\n" % (version[0], version[1], version[2])


it looks like this line is duplicated. I should remove one of them once this lands

arvidn · 2020-12-19T07:27:08Z

great! are these commits squashed to reasonable changes? (if not, please do that and I'll land it afterwards)

AllSeeingEyeTolledEweSew · 2020-12-20T11:39:44Z

great! are these commits squashed to reasonable changes? (if not, please do that and I'll land it afterwards)

I tried to keep them clean, but failed by the end. I squashed to one as it's pretty straightforward.

AllSeeingEyeTolledEweSew mentioned this pull request Dec 3, 2020

Build Python bindings and provide them as PyPI package #4218

Closed

3 tasks

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from c0ff27d to db655b0 Compare December 4, 2020 02:53

arvidn reviewed Dec 4, 2020

View reviewed changes

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from 7822727 to d195538 Compare December 7, 2020 02:24

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from d195538 to 4bcd247 Compare December 7, 2020 08:42

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch 3 times, most recently from 56245dc to c6a2c56 Compare December 7, 2020 09:51

AllSeeingEyeTolledEweSew mentioned this pull request Dec 7, 2020

Recover "python-libtorrent" package on pypi.org #5333

Closed

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from 282c8b3 to 0a36060 Compare December 9, 2020 15:26

arvidn reviewed Dec 10, 2020

View reviewed changes

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch 4 times, most recently from 10d30fd to ef1e2c0 Compare December 16, 2020 20:30

arvidn reviewed Dec 17, 2020

View reviewed changes

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from ef1e2c0 to 3420448 Compare December 17, 2020 22:09

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from 9c97626 to f97022c Compare December 17, 2020 22:51

arvidn reviewed Dec 19, 2020

View reviewed changes

arvidn approved these changes Dec 19, 2020

View reviewed changes

Better integration of setup.py to invoke b2

de9f8cb

AllSeeingEyeTolledEweSew force-pushed the asetes-setup.py-b2 branch from f97022c to de9f8cb Compare December 20, 2020 11:38

arvidn merged commit b87d012 into arvidn:RC_1_2 Dec 20, 2020

AllSeeingEyeTolledEweSew deleted the asetes-setup.py-b2 branch August 2, 2021 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better integration of setup.py to invoke b2 #5325

Better integration of setup.py to invoke b2 #5325

AllSeeingEyeTolledEweSew commented Dec 2, 2020

AllSeeingEyeTolledEweSew commented Dec 2, 2020 •

edited

Loading

arvidn commented Dec 3, 2020

AllSeeingEyeTolledEweSew commented Dec 3, 2020

arvidn commented Dec 3, 2020 •

edited

Loading

arvidn Dec 4, 2020

AllSeeingEyeTolledEweSew Dec 4, 2020

arvidn Dec 6, 2020

arvidn Dec 4, 2020

AllSeeingEyeTolledEweSew Dec 4, 2020

arvidn Dec 10, 2020

arvidn Dec 17, 2020

arvidn Dec 4, 2020

arvidn Dec 4, 2020

AllSeeingEyeTolledEweSew Dec 4, 2020

arvidn Dec 4, 2020

arvidn commented Dec 7, 2020

AllSeeingEyeTolledEweSew commented Dec 7, 2020

AllSeeingEyeTolledEweSew commented Dec 7, 2020

AllSeeingEyeTolledEweSew commented Dec 9, 2020

FranciscoPombal commented Dec 9, 2020

arvidn Dec 10, 2020

arvidn commented Dec 10, 2020

arvidn Dec 17, 2020

arvidn Dec 17, 2020

AllSeeingEyeTolledEweSew Dec 17, 2020

arvidn Dec 20, 2020

AllSeeingEyeTolledEweSew Dec 20, 2020

AllSeeingEyeTolledEweSew Dec 20, 2020

AllSeeingEyeTolledEweSew commented Dec 17, 2020

arvidn Dec 19, 2020

arvidn commented Dec 19, 2020

AllSeeingEyeTolledEweSew commented Dec 20, 2020

Better integration of setup.py to invoke b2 #5325

Better integration of setup.py to invoke b2 #5325

Conversation

AllSeeingEyeTolledEweSew commented Dec 2, 2020

AllSeeingEyeTolledEweSew commented Dec 2, 2020 • edited Loading

arvidn commented Dec 3, 2020

AllSeeingEyeTolledEweSew commented Dec 3, 2020

arvidn commented Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn commented Dec 7, 2020

AllSeeingEyeTolledEweSew commented Dec 7, 2020

AllSeeingEyeTolledEweSew commented Dec 7, 2020

AllSeeingEyeTolledEweSew commented Dec 9, 2020

FranciscoPombal commented Dec 9, 2020

Choose a reason for hiding this comment

arvidn commented Dec 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllSeeingEyeTolledEweSew commented Dec 17, 2020

Choose a reason for hiding this comment

arvidn commented Dec 19, 2020

AllSeeingEyeTolledEweSew commented Dec 20, 2020

AllSeeingEyeTolledEweSew commented Dec 2, 2020 •

edited

Loading

arvidn commented Dec 3, 2020 •

edited

Loading