-
Notifications
You must be signed in to change notification settings - Fork 308
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
Add github action to build python wheels (including python 3.9 and 3.10) #2097
Conversation
…of overloaded ‘abs(double)’ is ambiguous` Found with the manylinux_2_24_x86_64 gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
…e it's using pre C++11
… it should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a review as a walkthrough of changes
if [[ "$GITHUB_REF" == *"refs/tags"* ]]; then | ||
TWINE_REPOSITORY=pypi | ||
TWINE_PASSWORD=${{ secrets.PYPI_TOKEN }} | ||
else | ||
TWINE_REPOSITORY=testpypi | ||
TWINE_PASSWORD=${{ secrets.TESTPYPI_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To enable upload to PyPi and TestPyPi, you would need to add a repo secrets PYPI_TOKEN / TESTPYPI_TOKEN set to the token you get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I can do that when the time comes
echo "TWINE_REPOSITORY=$TWINE_REPOSITORY" >> $GITHUB_ENV | ||
echo "TWINE_PASSWORD=$TWINE_PASSWORD" >> $GITHUB_ENV | ||
|
||
twine upload wheels/*-manylinux*.whl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I need to tweak for sure
# Force C++11 since lambdas are used in CPStrings.h | ||
set(CMAKE_CXX_STANDARD 11) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to force this for mac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is fine. I'm looking forward to C++11.
return true; | ||
}; | ||
|
||
void set_phase() { | ||
double epsilon = 3.3e-5; // IAPWS-IF97 RMS saturated pressure inconsistency | ||
if ((abs(_T - IF97::Tcrit) < epsilon/10.0) && // RMS temperature inconsistency ~ epsilon/10 | ||
(abs(_p - IF97::Pcrit) < epsilon)) { // within epsilon of [Tcrit,Pcrit] | ||
if ((std::abs(_T - IF97::Tcrit) < epsilon/10.0) && // RMS temperature inconsistency ~ epsilon/10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to fix a couple of build errors on some platforms/compilers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one was "call to abs to ambiguous"
Fix build error: src/Backends/IF97/IF97Backend.h:54:34: error: call of overloaded ‘abs(double)’ is ambiguous
Found with the manylinux_2_24_x86_64
gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct solution I think
@@ -44,14 +45,14 @@ class IF97Backend : public AbstractState { | |||
this->_rhomass.clear(); | |||
this->_hmass.clear(); | |||
this->_smass.clear(); | |||
this->_phase = iphase_not_imposed; | |||
this->_phase = iphase_not_imposed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I have auto format for trailing spaces... view with ?w=1 in the URL to filter out whitespace)
elif os.environ.get('COOLPROP_CMAKE'): | ||
cmake_compiler, cmake_bitness = os.environ.get('COOLPROP_CMAKE').split(',') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow getting the cmake compiler and bitness from an env variable, so I can pass it to cibuildwheel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you need it in the build matrix?
elif cmake_compiler == 'vc16': | ||
cmake_build_args = ['--config', '"Release"'] | ||
if cmake_bitness == '32': | ||
cmake_config_args += ['-G', '"Visual Studio 16 2019"', '-A', | ||
'x86'] | ||
elif cmake_bitness == '64': | ||
cmake_config_args += ['-G', '"Visual Studio 16 2019"', '-A', | ||
'x64'] | ||
else: | ||
raise ValueError('cmake_bitness must be either 32 or 64; got ' + cmake_bitness) | ||
elif cmake_compiler == 'vc17': | ||
cmake_build_args = ['--config', '"Release"'] | ||
if cmake_bitness == '32': | ||
cmake_config_args += ['-G', '"Visual Studio 17 2022"', '-A', | ||
'x86'] | ||
elif cmake_bitness == '64': | ||
cmake_config_args += ['-G', '"Visual Studio 17 2022"', '-A', | ||
'x64'] | ||
else: | ||
raise ValueError('cmake_bitness must be either 32 or 64; got ' + cmake_bitness) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add MSVC 2019 and 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why we have "any" option, is this really needed? I recall having issues with this myself
cmake_build_string = ' '.join(['cmake', '--build', '.', '-j', | ||
str(cpu_count())] + cmake_build_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build with multiple cores.
@@ -448,6 +484,7 @@ def find_cpp_sources(root=os.path.join('..', '..', 'src'), extensions=['.cpp'], | |||
"Operating System :: OS Independent", | |||
"Topic :: Software Development :: Libraries :: Python Modules" | |||
], | |||
setup_requires=['Cython'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a failed attempt at getting cibuildwheel to install it. I guess I could remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would. It is needed to build, and the setup_requires
is a defective setuptools feature
# Window 32 bit | ||
- os: windows-latest | ||
python: 38 | ||
bitness: 32 | ||
platform_id: win32 | ||
cmake_compiler: vc17 | ||
arch: x86 | ||
allow_failure: true | ||
- os: windows-latest | ||
python: 39 | ||
bitness: 32 | ||
platform_id: win32 | ||
cmake_compiler: vc17 | ||
arch: x86 | ||
allow_failure: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two I can't get to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's ok, I had issues myself with 32-bit wheels. 32-bit windows is basically obsolete.
The CLA appears broken, I can't sign it Apparently the LICENSE_CLA.txt was deleted: 05daf20 |
Cf run on my fork: https://github.com/jmarrec/CoolProp/runs/5544908649?check_suite_focus=true TestedLinux
Mac, x86_64
Windows
Mac, arm64: failingTested on a M1 mac (Monterrey 12.2.1), doesn't work,
|
This pull request introduces 1 alert when merging a24fc20 into 381c853 - view on LGTM.com new alerts:
|
This is amazing. A few others have also talked about volunteering to help with the wheels but you just jumped in and did it. Excellent work. Looking forward to getting all tests working on all platforms. Could you please also build the shared libraries and the installer and the docs? If we get that all working, we can drop entirely our old CI system that nobody will be sorry to see go. |
@@ -0,0 +1,71 @@ | |||
name: Python Linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole file needs to get removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's disabled so no harm. Will clean it up later, once we're satisfied with the cibuildwheel
# Window 32 bit | ||
- os: windows-latest | ||
python: 38 | ||
bitness: 32 | ||
platform_id: win32 | ||
cmake_compiler: vc17 | ||
arch: x86 | ||
allow_failure: true | ||
- os: windows-latest | ||
python: 39 | ||
bitness: 32 | ||
platform_id: win32 | ||
cmake_compiler: vc17 | ||
arch: x86 | ||
allow_failure: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's ok, I had issues myself with 32-bit wheels. 32-bit windows is basically obsolete.
if [[ "$GITHUB_REF" == *"refs/tags"* ]]; then | ||
TWINE_REPOSITORY=pypi | ||
TWINE_PASSWORD=${{ secrets.PYPI_TOKEN }} | ||
else | ||
TWINE_REPOSITORY=testpypi | ||
TWINE_PASSWORD=${{ secrets.TESTPYPI_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I can do that when the time comes
# Force C++11 since lambdas are used in CPStrings.h | ||
set(CMAKE_CXX_STANDARD 11) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is fine. I'm looking forward to C++11.
return true; | ||
}; | ||
|
||
void set_phase() { | ||
double epsilon = 3.3e-5; // IAPWS-IF97 RMS saturated pressure inconsistency | ||
if ((abs(_T - IF97::Tcrit) < epsilon/10.0) && // RMS temperature inconsistency ~ epsilon/10 | ||
(abs(_p - IF97::Pcrit) < epsilon)) { // within epsilon of [Tcrit,Pcrit] | ||
if ((std::abs(_T - IF97::Tcrit) < epsilon/10.0) && // RMS temperature inconsistency ~ epsilon/10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct solution I think
elif os.environ.get('COOLPROP_CMAKE'): | ||
cmake_compiler, cmake_bitness = os.environ.get('COOLPROP_CMAKE').split(',') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you need it in the build matrix?
elif cmake_compiler == 'vc16': | ||
cmake_build_args = ['--config', '"Release"'] | ||
if cmake_bitness == '32': | ||
cmake_config_args += ['-G', '"Visual Studio 16 2019"', '-A', | ||
'x86'] | ||
elif cmake_bitness == '64': | ||
cmake_config_args += ['-G', '"Visual Studio 16 2019"', '-A', | ||
'x64'] | ||
else: | ||
raise ValueError('cmake_bitness must be either 32 or 64; got ' + cmake_bitness) | ||
elif cmake_compiler == 'vc17': | ||
cmake_build_args = ['--config', '"Release"'] | ||
if cmake_bitness == '32': | ||
cmake_config_args += ['-G', '"Visual Studio 17 2022"', '-A', | ||
'x86'] | ||
elif cmake_bitness == '64': | ||
cmake_config_args += ['-G', '"Visual Studio 17 2022"', '-A', | ||
'x64'] | ||
else: | ||
raise ValueError('cmake_bitness must be either 32 or 64; got ' + cmake_bitness) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why we have "any" option, is this really needed? I recall having issues with this myself
@@ -448,6 +484,7 @@ def find_cpp_sources(root=os.path.join('..', '..', 'src'), extensions=['.cpp'], | |||
"Operating System :: OS Independent", | |||
"Topic :: Software Development :: Libraries :: Python Modules" | |||
], | |||
setup_requires=['Cython'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would. It is needed to build, and the setup_requires
is a defective setuptools feature
I can certainly help. A quick bit of context first though: I actually don't really use CoolProp myself (I might have tried it a couple of times years ago). I work on a C++ project that exposes a python library, and I recently had to pin python to 3.8 become some users were using CoolProp and that wasn't available for 3.9+. So saw the linked issues and the call for help, so I decided to contribute because this 1) Looks like a cool OSS project, 2) I have knowledge of github actions, and 3) the cibuildwheel stuff was a nice exercise and one I hadn't tried before. I have burned quite a few hours already on this and I am not familiar with your build system (and looking at the CMakeLists.txt, it's a bit messy if I'm allowed to say (no judgment whatsoever)). So if you can chew what you want to happen or point me to existing build scripts that'd be nice, so I don't try to reivent the wheel. |
Sphinx docs-wise, they can be build with a docker container. The necessary is like this:
|
@jmarrec I know about the ugliness of the |
Just for fun I got another library of mine building all the wheels that matter: https://github.com/ibell/pdsim/blob/72592669c5ec3d1a824a80ce3c2a92854fe2aca2/.github/workflows/cibuildswheels.yml . Including the arm wheels on macos. So that makes me wonder: could we simplify the cibuildwheels stuff and make fixes to coolprop itself to avoid all custom things? |
Take that with a grain of salt (packaging external libs in C++ is something I've done but where I wouldn't claim any expertise) There is a large amount of custom stuff you have in setup.py, so it's possible some could be simplified. But this is still a native C++ lib built via CMake, so I'm not sure you can simply take all customization away. |
This was merged 11 days ago. Any ideas when we will see the result reflected in Pypi? As of now, pypi still only has up to version 6.4.1, which was uploaded Aug 4, 2020. |
Still a work in progress. I'm having a few kinks uploading to testpypi, should be resolved soon. https://github.com/CoolProp/CoolProp/runs/5976949824?check_suite_focus=true |
Awesome. |
@jmcarter17 Jean-Michel, you can now try Edit: I made an issue to centralize test results: #2119 |
Worked on Mac OSX (intel), with python 3.10.3 (pyenv) |
Ok, commented on the centralized test result #2119. |
It worked on Win10 64 with Python 3.10.2
|
…10) (CoolProp#2097) * Fix build error: `src/Backends/IF97/IF97Backend.h:54:34: error: call of overloaded ‘abs(double)’ is ambiguous` Found with the manylinux_2_24_x86_64 gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516 * Register MSVC 2019 and 2022 in setup.py * setup.py: when calling cmake, build in parallel * Enable using Env variables instead of passing them as args to setup.py * Github actions for linux: try 1 * use actions/checkout@v3 for submodules * mod setup.py:; typo * Random shot for cibuildwheel for all platforms * I thought package_dir was a flag, but it's positional * typo in cmake_compiler * add cython to setup_requires * try a pryproject.toml to install cython * try more requirements? * pywin32 only found on win32 I guess * Try with CIBW_BEFORE_BUILD instead * try to enable msvc via vcvarsall on windows, and pass MACOSX_DEPLOYMENT_TARGET * more tweaks for windoze * disable tests for now (fails on windows) * tweak mac again: it seems mac doesn't understand a C++ lambda, so like it's using pre C++11 * tweak * try 10.15 for mac... * try to force C++11 since mac picks up the path where lambdas are used... * Move back down to 10.9 now that C++11 is enabled and it works on mac, it should be enough * Try to debug win32 * Enable part of the upload step (minus the upload) to list the wheels * try to allow win32 to fail for now (instead of plain disabling) * Disable the python_linux.yml workflow, so cibuildwheels works fine. * Adjust the upload step to point to the right folder * make LGTM python happy
Description of the Change
Benefits
Python 3.9 and 3.10 aren't supported right now.
As far as I can tell there is also no real CI running right now (nor has been for a while) but I could be wrong. At least this is what this comment seem to indicate: #1981 (comment)
This sets up github actions to produce new python wheels for all platforms and automatically upload them to PyPi/TestPyPi
I also enabled arm64 mac builds (for M1 macs / Apple silicon)
Possible Drawbacks
Mac picks ups not
HAS_MOVE_SEMANTICS
but C++11 mode isn't enabled, so it chokes on lambdas. For now I generally enabled C++11, not sure if that's a big deal or not... Here it'll choke on line 45CoolProp/include/CPstrings.h
Lines 33 to 50 in 381c853
Verification Process
Right now I'm just looking at whether the CI build worked or not. I have not conducted extensive testing of whether the resulting wheels are valid or not.
Applicable Issues