Skip to content

Building wheels on CI #82

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

Merged
merged 231 commits into from
Mar 3, 2023
Merged

Building wheels on CI #82

merged 231 commits into from
Mar 3, 2023

Conversation

jncots
Copy link
Collaborator

@jncots jncots commented Oct 19, 2022

The main goal of this PR is to fix the problems which appear on CI when binary wheels are built using cibuildwheel. The main artifact is Release workflow in release.yml file which describes the process of building wheels and uploading them on PyPI.

The following problems were solve (in the order of their solution):

  • Building wheels for Windows. It is still not solved how to build Pythia8 and urqmd34 (see discussions below). So currently these models are excluded from the build. The main trick to solve the problem was adding the following lines in F2Py.cmake file:
if (WIN32)
    target_link_libraries(${target_name} PUBLIC "-static")
 endif()

Also there was a problem with setting correct generator in CMake and finding gfortran.

  • The main problem with Linux was that in Pythia8 there are 2 files: _pythia8 and libpythia8. _pythia8 depends on libpythia8. However, during build time _pythia8 obtained rpath, which doesn't allow to find libpythia8, if both files in the same directory. It was fixed by adding LINK_FLAGS "-Wl,-rpath... for _pythia8 target in CMakeLists.txt.

  • With MacOS there were 2 main problems: set correct version of gfortran and c++ and linking with Python library.

    • The solution of the problem with compilers was copied from https://github.com./numpy/numpy/tree/main/tools/wheels. gfortran_utils.sh script was copied from numpy to install the correct version of gfortran.

    • The problem with Python library was finally solved in PR avoid linking to Python #126

  • The last problem solved which is worth to mention is the finding a wrong python version via find_package(Python). The key issue was that cibuildwheel uses venv but do not activate it. Because of that find_package(Python) could not find python used in virtual environment. It is solved by setting environment variable VIRTUAL_ENV in setup.py

@jncots
Copy link
Collaborator Author

jncots commented Feb 15, 2023

This PR rewrites a workflow for Github Actions for building wheels on Windows, Linux, MacOS (x86-64 and arm64) and uploading it to PyPI. Since now wheels are being built for all platforms, the PR could be finally closed.

Copy link
Member

@afedynitch afedynitch left a comment

Choose a reason for hiding this comment

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

This is really quite a nerdy work. Thanks @jncots.

I'm approving this, however I would like to ask you to update the PR description since it contains stuff unrelated to what the PR is after the many months it was in the pipeline. Also you should rename it, since it is not about Windows. The description is part of the documentation and is really important to keep track of decisions made on the way.

@HDembinski, please also have a look if this is good to go.

@HDembinski
Copy link
Collaborator

HDembinski commented Feb 28, 2023

I added some comments, but generally this is good to go. Thanks @jncots for all this work.

@jncots
Copy link
Collaborator Author

jncots commented Mar 1, 2023

@afedynitch and @HDembinski the last 2 issues with this PR should be resolved. When PR will merge, it will try to upload built wheels on PyPI.

  • In the current state it will not be sucessful, because secrets.PYPI_API_TOKEN is not set. As I understand only @HDembinski can set it.
  • I am not sure that version = 0.3.0+dev will be accepted by PyPI. Should it be changed to something else?

If the current API is fine, then after resolving these issues, chromo can be finally uploaded to PyPI.

@jncots jncots changed the title Building wheels for Windows on CI Building wheels on CI Mar 1, 2023
@jncots
Copy link
Collaborator Author

jncots commented Mar 1, 2023

This is the old description of PR:

There were some problems that prevented to build IMPY on Windows.

CMake build system chose wrong toolchain and could not find fortran compiler. It is solved by choosing correct CMake generator. Setting environment variable CMAKE_GENERATOR=MinGW Makefiles solves the problem. On CI the action awvwgk/setup-fortran in workflow file installs gfortran that should be used.

Another problem is related also to generators on Windows. cmake_ext.py contains logic for MSVC generator which adds cmake_args += ["-A", arch] which prevents CMake configuration for MinGW and CMake doesn't work. Currently it is solved by commenting the line with cmake_args += ["-A", arch]. There should be correct approach. @HDembinski, what do you think is the way to fix this?

Next problem is related, probably, to restricted shell buffer for Windows. To build dpmjetIII191 CMake should execute the python script dpmjetIII191_workaround.py and to pass and accept a long list of files. On Windows there seems to be a problem with length of list. The solution to this is to pass the list of files via temporary file. CMakeLists.txt and dpmjetIII191_workaround.py are changed to a use temporary file if Windows is detected.

It turned out that after successful build all library files, the files themselves are not loaded. The attempt of loading gave "ImportError: DLL load failed: The specified module could not be found.". Dependency Walker showed that the libraries depended on gfortran and gcc dlls. The problem is solved by linking these library statically. In file F2Py.cmake the line target_link_libraries(${target_name} PUBLIC "-static") is added. It adds -static string to the end of linking command. In this case the library file is little bit larger but depends only on standard dlls.

Maybe there is a better way how to do that. For example add_link_options() function. I haven't tried.

Anyway, all models pass tests after building, except:

urqmd. It produces ImportError: DLL load failed %1 is not a valid win32 application. I will describe the problem in separate issue. Just mention here: when it is compiled withou O3 flag, the linking step produces multiple error of a kind "relocation truncated to fit: IMAGE_REL_AMD64_REL32 against". Maybe it is related to large static arrays.

@HDembinski
Copy link
Collaborator

HDembinski commented Mar 1, 2023

In the current state it will not be sucessful, because secrets.PYPI_API_TOKEN is not set. As I understand only @HDembinski can set it.

Right. I have to make a new token and register it. I will do that. @jncots and @afedynitch , please create accounts on PyPI if you haven't done so already. Tell me your account names. I will then add you as ownsers of the chromo package on PyPI, then you have full control.

Update: I created the token and registered it here on Github, so upload should work now.

@HDembinski
Copy link
Collaborator

I am not sure that version = 0.3.0+dev will be accepted by PyPI. Should it be changed to something else?

It should work, see https://semver.org/ for the rules. I think 0.3.0rc1 is more appropriate, though.

I am in favor of letting the package setuptools_scm manage the version automatically. I use this in some of my projects, and it is very convenient. setuptools_scm generates the version dynamically from the latest tag. This allows one to automate the release procedure so that a full release with upload to PyPI can be triggered by making a release on Github, which is setting a tag. I use the release trigger to make wheels and once they are finished, another job uploads the wheels to PyPI. The version in the wheels is automatically set via setuptools_scm, it will be the tag version. setuptools_scm also supports nightlies, it then uses <last release version>+dev<some commit>

@jncots jncots requested review from afedynitch and HDembinski and removed request for HDembinski and afedynitch March 2, 2023 04:45
@jncots
Copy link
Collaborator Author

jncots commented Mar 3, 2023

@HDembinski, @afedynitch I going to merge this PR in an hour, so if everything will be go smooth, chromo will uploaded on PyPI. I think introduction of setuptools_scm is for another PR.

@HDembinski
Copy link
Collaborator

@HDembinski, @afedynitch I going to merge this PR in an hour, so if everything will be go smooth, chromo will uploaded on PyPI. I think introduction of setuptools_scm is for another PR.

Yes, that was my intend, I was not suggesting to add setuptools_scm to this PR. Again, great job @jncots . The solution you found for packaging shared libs will also come handy in other projects.

@jncots jncots merged commit 0238a08 into main Mar 3, 2023
@jncots jncots deleted the gh_ci branch March 3, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants