Skip to content

avoid linking to Python #126

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 2 commits into from
Feb 10, 2023
Merged

avoid linking to Python #126

merged 2 commits into from
Feb 10, 2023

Conversation

HDembinski
Copy link
Collaborator

This seems to avoid the linking to Python, which is causing issues in #82

@HDembinski HDembinski mentioned this pull request Feb 9, 2023
@HDembinski
Copy link
Collaborator Author

I removed the mold code here after realizing a) the mold linker flags were actually never set, because the if conditions never triggered, b) the linker flag -ld-path=<path to mold> does not work when you link with gfortran.

@HDembinski HDembinski requested a review from afedynitch February 9, 2023 17:27
@HDembinski
Copy link
Collaborator Author

This should be merged into main and then into #82 to remove the need for fix_macos_installation and a couple of other workarounds.

@afedynitch
Copy link
Member

This looks really elegant, thanks. I'm wondering how did you find this solution?

Copy link
Collaborator

@jncots jncots left a comment

Choose a reason for hiding this comment

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

Thank you for the solution!

@jncots jncots merged commit 736cb70 into main Feb 10, 2023
@jncots jncots deleted the avoid_linking_to_python branch February 10, 2023 05:18
@HDembinski
Copy link
Collaborator Author

HDembinski commented Feb 10, 2023

This looks really elegant, thanks. I'm wondering how did you find this solution?

You can follow my thoughts in the comments to #82 , but my general advice is: if you encounter a problem, study the code of scipy or numpy or pybind11, which deal with similar issues as we do and check what they are doing. These three software packages have expertly written build systems, commits are reviewed by experts with a lot of knowledge, so the solutions which end up in those packages tend to be very good. Code documentation is also often very good.

In this particular case, I checked what the macro pybind11_add_module is calling to build the Python module.

@afedynitch
Copy link
Member

These three software packages have expertly written build systems, commits are reviewed by experts with a lot of knowledge, so the solutions which end up in those packages tend to be very good. Code documentation is also often very good.

I checked scipy and numpy but they all use meson for a while 😛

@eli-schwartz
Copy link

I checked scipy and numpy but they all use meson for a while stuck_out_tongue

The meson handling calls out to the meson "python" support module's py.dependency() method, which defaults to embed: false. This roughly corresponds to e.g. the python pkg-config file's split between python.pc and python-embed.pc (the latter is used for producing executables that load a python interpreter, the former for extension modules).

There's platform-specific handling here, it's not so simple as "don't link to libpython", but that's mostly a difference between Windows (do link) and everywhere else (don't link, starting with python 3.8).

@HDembinski
Copy link
Collaborator Author

HDembinski commented Feb 13, 2023

We use the cmake command Python_add_library now, which is supposed to handle this in a platform independent way. Since you say it is complicated, I assume they (cmake) added this to abstract all these details away for us.

jncots added a commit that referenced this pull request Mar 3, 2023
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 #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

---------

Co-authored-by: Hans Dembinski <[email protected]>
Co-authored-by: Hans Dembinski <[email protected]>
Co-authored-by: afedynitch <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com.>
Co-authored-by: Anatoli Fedynitch <[email protected]>
Co-authored-by: Henry Schreiner <[email protected]>
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