Skip to content
This repository has been archived by the owner on Aug 23, 2024. It is now read-only.

Add CI for a Python wheel release to PyPi #72

Merged
merged 13 commits into from
Apr 14, 2021

Conversation

mballance
Copy link
Contributor

Signed-off-by: Matthew Ballance matt.ballance@gmail.com

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
@mballance
Copy link
Contributor Author

I've updated the Azure Pipelines specification to only authenticate and upload to PyPi when the build is done from the 'master' branch. It's pretty unlikely that we want PR builds to make uploads to PyPi.

In order to enable automated uploads to pypi.org, the following must be done in the Azure Pipelines Builds section. Before doing so, register for a free account on pypi.org

  • Project Settings -> Service Connections (it's under the Pipelines heading)
  • New Service connection -> Python package upload
    • Connection name: pypi
    • Python repository url for upload: https://pypi.python.org/pypi
    • EndpointName: pyboolector
    • Username: pypi.org username
    • Password: pypi.org password
  • Select OK

The CI script can now authenticate a connection to PyPi.org via the information saved in the service connection configuration.

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
@mpreiner
Copy link
Member

mpreiner commented Dec 3, 2019

@mballance Thanks a lot for the work and sorry for the delay! We'll have a look and setup pypi ASAP.

@mpreiner
Copy link
Member

@mballance Sorry that this takes so long. I had a brief look (will dive into it more soon). However, I have one question: Why did you choose to use Apache instead of sticking with MIT that comes with Boolector? This may cause some confusion since only the code for packaging pyboolector for pypi would be under Apache, but Boolector and pyboolector are still MIT, which is currently not reflected in setup.py.

@mballance
Copy link
Contributor Author

@mpreiner, looks like an oversight on my part. I'll be happy to correct the license to align with the MIT that Boolector uses.

-Matthew

@mpreiner
Copy link
Member

Thanks @mballance!

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
Copy link
Member

@mpreiner mpreiner left a comment

Choose a reason for hiding this comment

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

@mballance as discussed offline here is the first pass over the PR. It seems that the build script can be simplified quite a bit and I'd like to include CaDiCaL, which is the default SAT solver for Boolector and provides better performance. Thanks again for your work and sorry for the delay!

pypi/LICENSE Outdated Show resolved Hide resolved
pypi/build.sh Outdated Show resolved Hide resolved
pypi/build.sh Outdated Show resolved Hide resolved
pypi/build.sh Outdated Show resolved Hide resolved
pypi/build.sh Show resolved Hide resolved
pypi/build.sh Outdated Show resolved Hide resolved
pypi/build.sh Outdated Show resolved Hide resolved
pypi/build.sh Show resolved Hide resolved
…ge ; Update lingeling commit hash

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
@mballance
Copy link
Contributor Author

Hi @mpreiner,
I've updated the PR with your requested changes, and also added in a couple of changes from the separate CI build -- macOS release, and improved versioning.

Thanks and Best Regards,
Matthew

@aytey
Copy link
Contributor

aytey commented Mar 14, 2021

My Lingeling PR isn’t merged; how are you using the commit hash from that?

Could we do something here like we do with the Windows patches to make this self-contained within the Boolector repo?

pypi/build.sh Outdated Show resolved Hide resolved
@mballance
Copy link
Contributor Author

My Lingeling PR isn’t merged; how are you using the commit hash from that?

Could we do something here like we do with the Windows patches to make this self-contained within the Boolector repo?

Apologies for the confusion, @andrewvaughanj. I wanted to point @mpreiner to a description of the issue resolved by moving to the latest lingeling code. It appears the issue has been resolved independent of your PR. The clone variable is now named 'cloned'.

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
@aytey
Copy link
Contributor

aytey commented Mar 14, 2021

My Lingeling PR isn’t merged; how are you using the commit hash from that?
Could we do something here like we do with the Windows patches to make this self-contained within the Boolector repo?

Apologies for the confusion, @andrewvaughanj. I wanted to point @mpreiner to a description of the issue resolved by moving to the latest lingeling code. It appears the issue has been resolved independent of your PR. The clone variable is now named 'cloned'.

Ah! All good :)

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>
Copy link
Member

@mpreiner mpreiner left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes! A few more comments before we are good to go.

pypi/setup.py Show resolved Hide resolved
pypi/setup.py Outdated Show resolved Hide resolved
pypi/setup.py Outdated Show resolved Hide resolved
Comment on lines +51 to +66
for py in cp35-cp35m cp36-cp36m cp37-cp37m cp38-cp38 cp39-cp39; do
echo "Python: ${py}"
python=/opt/python/${py}/bin/python
cd ${BUILD_DIR}/pyboolector
rm -rf src
cp -r ${BUILD_DIR}/boolector/src/api/python src
sed -i -e 's/override//g' \
-e 's/noexcept/_GLIBCXX_USE_NOEXCEPT/g' \
-e 's/\(BoolectorException (const.*\)/\1\n virtual ~BoolectorException() _GLIBCXX_USE_NOEXCEPT {}/' \
src/pyboolector_abort.cpp
mkdir -p src/utils
cp ${BUILD_DIR}/boolector/src/*.h src
cp ${BUILD_DIR}/boolector/src/utils/*.h src/utils
$python ./src/mkenums.py ./src/btortypes.h ./src/pyboolector_enums.pxd
$python setup.py sdist bdist_wheel
done
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity: Why can't the library produced by ./configure --python be used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way I could find to produce a 'wheel' (Python distribution package) that PyPi would accept was to use the Python build commands via setup.py. I think part of this is due to the package auditing that exists to ensure that packages posted on PyPi will truly be compliant with the platform specifications they purport to be compatible with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, one option going forward (thinking Bitwuzla) would be to build all Python packages via a setup.py. The CMakeLists.txt would be slightly different, and would invoke Python on the setup.py instead of directly building the shared library. The advantage would be a single way of building the Python package.

Copy link
Member

@mpreiner mpreiner Mar 26, 2021

Choose a reason for hiding this comment

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

Oh ok. Do you have experience with scikit-build? Looks like this is allows better integration with a cmake build system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't. I think I looked at scikit-build early in my process of learning about building Python extensions, and think my conclusion was that it was optimized for certain types of extension but lacked flexibility. Certainly not in a position to offer a knowledgeable opinion, though.

I've been working on building Python extensions as part of the regular build in the context of a different project. Thus far, it doesn't look too difficult. Here's the CMakeList fragment that invokes the actual build:
https://github.com/PSSTools/pssparser/blob/67fdb8d0cd41874f80f6e8c1acb60a83ac6cc156/ext/CMakeLists.txt#L7-L20

This, in turn, is driven by a script run inside the 'manylinux2010' Docker image to produce all the Python-version variants:
https://github.com/PSSTools/pssparser/blob/67fdb8d0cd41874f80f6e8c1acb60a83ac6cc156/.github/workflows/pypi.sh#L26-L38

Fortunately, Makefile dependencies cause only the Python extension (and not all the other code) to be rebuilt for each Python version

sed -i -e 's/override//g' \
-e 's/noexcept/_GLIBCXX_USE_NOEXCEPT/g' \
-e 's/\(BoolectorException (const.*\)/\1\n virtual ~BoolectorException() _GLIBCXX_USE_NOEXCEPT {}/' \
src/pyboolector_abort.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this bit for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to support a broad range of Linux distributions, pre-compiled Python libraries are built on specific older Linux distros with older compilers (Python defines a manylinux docker image based on CentOS). The supported compiler versions have limits on the C++ dialects supported (override, noexcept as a keyword, etc). This filter patches the code to comply with the compiler-supported C++.

@@ -0,0 +1,113 @@

jobs:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to only run the upload to pypi if we do a new release? I think PyPi has upload limits and we should not upload a new version on every commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be, as long as we can detect creation of a tag or something else that marks the creation of a release. I always find I have to do a bit of Googling and playing around to get this to work, but there typically is a way. How are releases marked for Boolector? A tag? A branch?
That said, I've not detected any upload limits with PyPi on any of the projects I'm using it with.

Copy link
Member

@mpreiner mpreiner Mar 26, 2021

Choose a reason for hiding this comment

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

Releases are tagged, but there is also the releases page: https://github.com/Boolector/boolector/releases

If Azure doesn't have this kind of functionality we could also think of switching the PyPi upload to GitHub actions, which supports running workflows only for specific actions such as creating a release.

About the size limit: I was also not aware of that, but it was mentioned here: cvc5/cvc5#5657 (comment)

Copy link
Member

@mpreiner mpreiner left a comment

Choose a reason for hiding this comment

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

@mballance is there anything else to do other than what you wrote here once the PR is merged?

@mballance
Copy link
Contributor Author

@mballance is there anything else to do other than what you wrote here once the PR is merged?

@mpreiner, other than configuring PyPi upload (referenced link), we should be good to go. Please let me know the PyPi username that will be doing uploads, since I think I'll need to add it to the PyPi project.

@mpreiner mpreiner merged commit 4a46a95 into Boolector:master Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants