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

Factor out C-API, Python API and solver tests into their own crates #80

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

chrjabs
Copy link
Owner

@chrjabs chrjabs commented Apr 4, 2024

Something about dependency resolution makes this break on macos and windows but not on Linux.

@ChrisJefferson
Copy link
Contributor

This is just some uninformed advice, based on hitting a similar issue (you can see people talking about it here: rust-lang/cargo#6313 )

Basically, I think, cdylibs don't work well with packages that get built multiple times, and rustsat is now built with different features in different places.

The best (again, I think) solution would be to pull the C and Python interface out into their own little package, which requires rustsat. Nothing depends on that interface package, and that should "break the cycle" (although as I say, I'm guessing here).

You could try removing cdylib and see if that at least breaks the set of broken tests, to see if that moves things in the right direction?

@chrjabs
Copy link
Owner Author

chrjabs commented Apr 5, 2024

Thanks for the pointers, that might come in very handy. So far I wanted to keep the Python API in the main crate so that it can follow the main versioning more easily, but this might have to change then.

@chrjabs
Copy link
Owner Author

chrjabs commented Apr 7, 2024

Looks like the cdylib is indeed the problem. I guess I'll have to factor out the external APIs indeed then. At least that seems like the cleanest solution.

rustsat/src/encodings/pb/dpw.rs Outdated Show resolved Hide resolved
pyapi/src/instances.rs Outdated Show resolved Hide resolved
pyapi/src/types.rs Outdated Show resolved Hide resolved
pyapi/src/types.rs Show resolved Hide resolved
pyapi/src/types.rs Outdated Show resolved Hide resolved
@chrjabs chrjabs changed the title Factor out solver tests into their own crate to be able to reuse them Factor out C-API, Python API and solver tests into their own crates Apr 9, 2024
@chrjabs
Copy link
Owner Author

chrjabs commented Apr 10, 2024

To-dos before merging:

  • Ignore rustsat-pyapi in release-plz
  • Factor out solver unit tests into rustsat-solvertests

factor out C-API into separate crate and tooling around that
@chrjabs chrjabs marked this pull request as ready for review April 10, 2024 13:08
@chrjabs
Copy link
Owner Author

chrjabs commented Apr 10, 2024

More To-dos:

  • Add basic tests to the python API

factor out Python API into separate crate and tooling around that
- include minisat and glucose as submodules
- specify linked libraries in code rather than in the build script
@chrjabs chrjabs merged commit 78334a7 into main Apr 11, 2024
44 checks passed
@chrjabs chrjabs deleted the refactor/solver-tests branch April 11, 2024 18:57
@chrjabs chrjabs added this to the Version 0.5.0 milestone Apr 11, 2024
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.

2 participants