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

pycryptosat doesn't build #763

Open
antonio-rojas opened this issue Jul 14, 2024 · 12 comments
Open

pycryptosat doesn't build #763

antonio-rojas opened this issue Jul 14, 2024 · 12 comments

Comments

@antonio-rojas
Copy link
Contributor

pycryptosat doesn't build in 5.11.22 as it's using API that was removed from the C++ library

python/src/pycryptosat.cpp: In function ‘PyObject* start_getting_small_clauses(Solver*, PyObject*, PyObject*)’:
python/src/pycryptosat.cpp:232:18: error: ‘class CMSat::SATSolver’ has no member named ‘start_getting_small_clauses’
  232 |     self->cmsat->start_getting_small_clauses(max_len, max_glue);
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
python/src/pycryptosat.cpp: In function ‘PyObject* get_next_small_clause(Solver*, PyObject*, PyObject*)’:
python/src/pycryptosat.cpp:250:29: error: ‘class CMSat::SATSolver’ has no member named ‘get_next_small_clause’
  250 |     bool ret = self->cmsat->get_next_small_clause(lits);
      |                             ^~~~~~~~~~~~~~~~~~~~~
python/src/pycryptosat.cpp: In function ‘PyObject* end_getting_small_clauses(Solver*, PyObject*, PyObject*)’:
python/src/pycryptosat.cpp:281:18: error: ‘class CMSat::SATSolver’ has no member named ‘end_getting_small_clauses’
  281 |     self->cmsat->end_getting_small_clauses();
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~
@msoos
Copy link
Owner

msoos commented Jul 14, 2024

Thanks, good point. Sorry, I was a bit under the weather. I'm fixing this now.

@msoos
Copy link
Owner

msoos commented Jul 14, 2024

Hi,

I have now pushed a new version to pypi:

https://pypi.org/project/pycryptosat/#files

You can check out how to build it by hand here:

https://github.com/msoos/cryptominisat/blob/master/.github/workflows/python-wheel-build.yml

I hope this resolves your issue!

Mate

@msoos
Copy link
Owner

msoos commented Jul 14, 2024

Actually, the built python module is wrong. I don't know how to fix this. I think I'm giving up. No more python module. Sorry.

@dimpase
Copy link
Contributor

dimpase commented Jan 12, 2025

Should this be re-opened?

@msoos
Copy link
Owner

msoos commented Jan 12, 2025

Hi, this has been closed. I have no idea how to build the damned module. I can build the .so libraries, and it works on my computer. But I have no idea how to package these into a single library using pypi. I know this sounds bad. But I spent weeks, maybe even a month or two trying to do this thing and I am exhausted fighting with the build system of pypi. I can make it compile and run on my machine, but the wheel it builds only installs one of the .so files, and so all the dependencies are missing when the wheel is installed by anyone else. If you know how to fix this, please help. My understanding is that it's possible to fix via nix, but I just ran out of steam.

I hope this helps explain why I am not creating a python module,

Mate

@dimpase
Copy link
Contributor

dimpase commented Jan 12, 2025

I am not asking you to spend more of your time on this, I merely ask for this to remain open - unless you really won't consider a PR which will fix this issue.

With integrated with cmake tools like pybind11, allowing one to build Python interfaces to C++ libraries, this can't be too hard to fix.

@antonio-rojas
Copy link
Contributor Author

fwiw I fixed this downstream by patching setup.py to build against the shared cryptominisat instead of building its own static copy. This should work for distro packaging, no idea about pypi

https://gitlab.archlinux.org/archlinux/packaging/packages/cryptominisat/-/blob/main/python-system-libs.patch?ref_type=heads

@msoos
Copy link
Owner

msoos commented Jan 12, 2025

Wow nice! Yeah, I have been doing something like what you have done in that PR as well -- tried building it shared. And that works beautifully! But it doesn't work in a system like pypi :S

Unfortunately the issue isn't binding, i.e. pybind11 won't help much I think :( The issue is linking, at least for pypi. The system works, AFAIK, if you build it locally. Reopening :)

@msoos msoos reopened this Jan 12, 2025
@msoos
Copy link
Owner

msoos commented Jan 12, 2025

Actually, @antonio-rojas do you wanna create a PR out of that changeset? I'd merge it right in :)

@antonio-rojas
Copy link
Contributor Author

I don't think my patch is suitable to be merged as-is (otherwise I would have submitted a PR already) - it assumes that the library has already been built, and the path specified in library_dirs needs to match the build dir used by cmake. Ideally the shared library build should be integrated in setup.py (or the cmake build should have an option to build the python bindings) in such a way that the library build dir can be communicated properly to the python module build.

@msoos
Copy link
Owner

msoos commented Jan 12, 2025

Yeah, good point, building it as part of cmake would make sense. Then it would "just work". Maybe there are cmake modules that can do that.

@dimpase
Copy link
Contributor

dimpase commented Jan 12, 2025

https://scikit-build-core.readthedocs.io/en/latest/ is the current recommendation for cmake-powered building of python packages. It's used by scikit, (py)zeromq, and many other very popular python projects.

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

No branches or pull requests

3 participants