-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add WebAssembly/TypeScript bindings #5762
Conversation
|
@@ -1,43 +1,51 @@ | |||
name: WASM Build | |||
name: WebAssembly Build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this replaces the old action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as mentioned in the PR message. The old one didn't do much useful - you couldn't use the artifact without having the same version of the compiler used to build it.
@@ -75,6 +75,13 @@ src/api/ml/z3enums.ml | |||
src/api/ml/z3native.mli | |||
src/api/ml/z3enums.mli | |||
src/api/ml/z3.mllib | |||
src/api/js/node_modules/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why they are in .gitignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the usual practice for JS projects. This folder holds the dependencies used to build the project.
- name: Publish | ||
run: npm publish | ||
env: | ||
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have installed an NPM token with my account that has write access to z3-solver.
I deleted the z3-solver organization.
|
||
- name: Prepare for publish | ||
run: | | ||
npm version $(node -e 'console.log(fs.readFileSync("../../../scripts/release.yml", "utf8").match(/ReleaseVersion:\s*\x27(\S+)\x27/)[1])') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the version without "-pre" because I can't find a reference to "-pre" in the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I deleted it just before opening this PR. Sorry that wasn't clearer.
let types = { | ||
__proto__: null, | ||
|
||
// these are function types I can't be bothered to parse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will likely create other such callbacks so if this list is required the more sustainable way is to mark them in z3_api as callbacks using some no-op macro. Based on the code below it looks like you match for "typedef enum" and other typedefs are not considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added macros so the closures can be discovered instead of hard-wired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add support for parsing these properly, but to be sure I've done it right I'd like to have at least a test of them. Right now I can't find any uses of any of these types to compare against. Can you write a brief example (in C/C++) demonstrating how any one of the consumers of the Z3_whatever_eh
types is intended to work, such that I could tell if my implementation has the right behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 212 in 56d3718
self.eh = Z3_set_error_handler(self.ctx, z3_error_handler) |
Other callbacks are not exercised yet in examples I have published for python. They are used from C++
Line 3966 in 56d3718
static void pop_eh(void* p, unsigned num_scopes) { |
and then for a self-contained example:
https://github.com/Z3Prover/z3/tree/master/examples/userPropagator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/Z3Prover/z3/tree/master/examples/userPropagator looks like something I can re-implement, thanks. That PDF is very helpful.
For Z3_set_error_handler
, what's an example of a case where it should be called? For example, should I expect it to be called if I perform Z3_query_constructor
on a Z3_constructor
which has not previously been passed to Z3_mk_datatypes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the error handler gets invoked whenever there is some mistake in the API call.
For example, if you add two arrays. The addition is not well typed and it fails to create the expression.
Without the error handler it should return null. With the error handler you can raise an exception from the handler assuming stack unwinding works (which I guess for WebAssembly it should).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question: this line:
Line 6700 in 56d3718
* The registered function appears at the top level and is created using \ref Z3_propagate_solver_declare. |
refers to a Z3_propagate_solver_declare
. I can't find any other mention of that. Is it just a typo?
// but that's super painful | ||
// and the files are regular enough that we can get away without it | ||
|
||
// we could also do this by modifying the `update_api.py` script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see adding things to update_api as a goal. It doesn't serve a purpose in itself. At best one can reuse some feature from update_api, but this seems needless now.
The goals are to ensure the bindings can be built on every push and the bindings can be exported (to npm).
|
||
// we _could_ use an actual C++ parser, which accounted for macros and everything | ||
// but that's super painful | ||
// and the files are regular enough that we can get away without it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files have to be regular.
Thanks for merging! You should try the new wasm-publish action to confirm you can publish the package. |
This is to ease integration with external API wrappers that rely on accessing information about type names that are used. #5762
As discussed. See the files named
src/js/api/README.md
andsrc/js/api/PUBLISHED_README.md
for more details.This includes two new GitHub Actions:
I confirmed these work on my fork.
The version number for the release is read (with a regex) from
release.yml
.In order to use the publish action, I'll need to add your npm account as a maintainer of the package, and then you'll need to create an
automation
token and add it as a secret for the repository namedNPM_TOKEN
. Please let me know the account you'd like to use.npm will not let you publish over an existing version, but I published the version from my fork as
-pre
. So you should be able to trigger the publish job once this lands.also, cc @cpitclaudel @philzook58 @bramvdbogaerde as people who have previously expressed interest in having wasm bindings.