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 OpenSMT to version 2.8.0 #426

Merged
merged 5 commits into from
Jan 11, 2025
Merged

Update OpenSMT to version 2.8.0 #426

merged 5 commits into from
Jan 11, 2025

Conversation

daniel-raffler
Copy link
Contributor

Hello everyone,
this pull request will update OpenSMT to version 2.8.0. We're currently using a fairly recent version of OpenSMT 2.6.X and there have been few larger changes since. However, since there was some internal refactoring, and all the code has been moved into the opensmt namespace, some changes to the SWIG headers are needed. We use this opportunity to also update out own build scripts and will now include all the auto-generated SWIG code in the repository. This is already done for Bitwuzla and should make it easier for us to spot any unintended changes when the JNI bindings are again updated. In addition to that we changed the build process to only produce a single library that can be included more easily from Java and doesn't require us to patch the rpath.

Since this update isn't too urgent we might want to wait for #395 before committing it.

Comment on lines +165 to +166
<arg value="/dependencies/gmp-6.2.1/.libs/libgmp.a"/>
<arg value="/dependencies/gmp-6.2.1/.libs/libgmpxx.a"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These paths will only work when using the docker image to build the bindings. We could use a property -Dopensmt.gmp.path=XXX to let the user set the path. Alternatively we could also download (and build) GMP in the ant script itself.

Copy link
Member

@kfriedberger kfriedberger Jan 11, 2025

Choose a reason for hiding this comment

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

Personally, I do not like using dependencies from the Docker image. Other solvers include GMP from separate build directories, where the developer can exchange the files directly. The only benefit is a "constant" version from the Docker image.
Using an option would be a good step towards a more flexible solution.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it works quite well for now. I see no problem in mergin this and improving it later.

Comment on lines +80 to +82
// TODO These were also added recently. Are they useful to us?
%ignore MainSolver::printResolutionProofSMT2() const;
%ignore MainSolver::printResolutionProofSMT2(std::ostream &) const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If these are interesting to use they can still be included

Copy link
Member

Choose a reason for hiding this comment

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

It does not hurt to have a full API available. Then it is simpler to add new features later.

Copy link
Member

@kfriedberger kfriedberger left a comment

Choose a reason for hiding this comment

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

Looks good overall.

I will build OpenSMT, test this branch, and give feedback.

@@ -0,0 +1,416 @@
diff --git a/lib/native/source/opensmt/opensmt-wrap.cpp b/lib/native/source/opensmt/opensmt-wrap.cpp
Copy link
Member

Choose a reason for hiding this comment

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

OpenSMT is licensed with MIT license.
Should we publish all the JavaSMT bindings for OpenSMT as Apache2.0 OR MIT, or only the C files?

@@ -0,0 +1,416 @@
diff --git a/lib/native/source/opensmt/opensmt-wrap.cpp b/lib/native/source/opensmt/opensmt-wrap.cpp
index cfe601cd2..70df21180 100644
Copy link
Member

Choose a reason for hiding this comment

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

This file is funny: a patch file for license headers, and it has a separate license file for itself.

Good idea and valid solution.

PS: You could also have created a separate license file FILE.i.license per source file FILE.i.

@kfriedberger kfriedberger self-assigned this Jan 11, 2025
Copy link
Member

@kfriedberger kfriedberger left a comment

Choose a reason for hiding this comment

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

Seems to work. The PR is well-written and the single-file library is a great improvement.
I will publish the native library in Ivy and Maven and merge this PR.

Ubuntu 18.04 does not provide the required compiler version for c++20, so we use Ubuntu 22.04.
@kfriedberger kfriedberger merged commit fd322fa into master Jan 11, 2025
2 of 3 checks passed
@kfriedberger kfriedberger deleted the update-openSMT-to-2.8 branch January 11, 2025 22:01
@daniel-raffler
Copy link
Contributor Author

Thanks for merging! I'll keep your comments in mind for the next OpenSMT update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants