Skip to content
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

Update repository for conan2 #365

Merged
merged 27 commits into from
Jun 8, 2023
Merged

Update repository for conan2 #365

merged 27 commits into from
Jun 8, 2023

Conversation

Jean1995
Copy link
Member

@Jean1995 Jean1995 commented Apr 20, 2023

This PR updates the conanfile, the installation instructions, setup.py, and the github actions scripts to be compatible with conan2.

The old installation approach, which relied on the cmake_paths generator of conan, has been deprecated.
Instead, I have oriented myself on the procedure presented in the conan2 tutorial, which uses the approach where conan creates a CMake toolchain file which tells CMake where to find the packages installed by conan. This has the advantage that the conan installation process is completely separated from the rest of our CMake structure.

The installation procedure using conan and CMake now has a few more steps, but is more transparent for the user.

pip install proposal still works ous of the box. With the update to conan2, a lot of issues that required manual intervention by us (for example the libcxx settings) have been resolved, which also simplified the setup.py.

The description of the installation process as well as the setup.py are not backwards-compatible with conan 1.x.
Therefore I gave pip install "conan~=2.0" as an explicit instruction in the INSTALL.md for now.

Other changes

  • For the python CI, I have added a macOS runner, because in the past, we had problems with the pip installation exclusive to MacOS

@Jean1995 Jean1995 added this to the 7.6.0 milestone Apr 20, 2023
@Jean1995 Jean1995 requested review from maxnoe and The-Ludwig April 20, 2023 12:39
@maxnoe
Copy link
Member

maxnoe commented Apr 20, 2023

Might this be a good point to also try to get rid of the custom cmake code in setup.py and try if we can use scikit-build or even scikit-build-core?

if self.options.with_python:
self.requires("pybind11/2.10.1")
if self.options.with_testing:
self.requires("boost/1.78.0")
self.requires("boost/1.75.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

why the downgrading from 1.78 to 1.75

Copy link
Member Author

Choose a reason for hiding this comment

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

cubicinterpolation itself has a requirement of 1.75.0, and somehow this creates a conflict in conan when proposal defined a different version.
However, this is only relevant if one wants to build the tests. It shouldn't make any practical difference.

@Jean1995
Copy link
Member Author

Might this be a good point to also try to get rid of the custom cmake code in setup.py and try if we can use scikit-build or even scikit-build-core?

Since we are touching the setup.py anyway, now would be the right time if we want to improve it. However, I've never worked with the scikit-build generator so far.

INSTALL.md Outdated Show resolved Hide resolved
from conan.tools.microsoft import is_msvc
from conan.tools.scm import Version

required_conan_version = ">=1.53.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure: this conanfile is still compatible with conan 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. At the moment, the conanfile is written in such a way that it is backwards-compatible with conan 1.x.

@The-Ludwig
Copy link
Contributor

Might this be a good point to also try to get rid of the custom cmake code in setup.py and try if we can use scikit-build or even scikit-build-core?

Since we are touching the setup.py anyway, now would be the right time if we want to improve it. However, I've never worked with the scikit-build generator so far.

I never worked with compiled parts of a python package, but I wonder how conan integrates into scikit-build? Wouldn't PROPOSAL still need custom setup.py code for conan? Interesting thread I found on this: https://github.com/orgs/scikit-build/discussions/645

@maxnoe
Copy link
Member

maxnoe commented Apr 21, 2023

Maybe also open an issue about a possible conan integration in the new scikit-build-core project? They are rewriting from scratch and handling the C++ dependency is probably important for more people:
https://github.com/scikit-build/scikit-build-core/issues

@The-Ludwig
Copy link
Contributor

Maybe also open an issue about a possible conan integration in the new scikit-build-core project? They are rewriting from scratch and handling the C++ dependency is probably important for more people: https://github.com/scikit-build/scikit-build-core/issues

Maybe it's better if you open an issue, since I have no idea about scikit-build and C++ distribution through pip and only used conan in one project :D

@Jean1995
Copy link
Member Author

Jean1995 commented May 2, 2023

How to we want to proceed with the scikit-build(-core) suggestion?
Open an issue in their repository asking about conan, and (for now) use the custom cmake solution?

@The-Ludwig
Copy link
Contributor

How to we want to proceed with the scikit-build(-core) suggestion? Open an issue in their repository asking about conan, and (for now) use the custom cmake solution?

I would do so, unless you we want this to be a pilot-project for a custom conan+scikit-build workflow...

@Jean1995
Copy link
Member Author

How to we want to go forward with this MR? This is the last issue I would like to solve for the next release.

Does this concept work for you?

@Jean1995 Jean1995 merged commit 08b6129 into master Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants