-
Notifications
You must be signed in to change notification settings - Fork 444
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
Build Z3 from source instead of downloading precompiled binaries. #4697
Conversation
5ba0346
to
156b925
Compare
I have no urgency for this issue, but if I understand correctly that with this change, then building p4c will by default fetch z3 source code of a particular desired version and build it from source code, then as long as that version also compiles and works for arm64 processors (e.g. Apple Silicon running arm64/aarch64 Linux in a VM), it would slightly simplify the work of those building P4 tools for that processor ach. |
Yes! It also allows us to finally upgrade Z3 in tools and fix some of the segmentation fault problems. But there is still some work to do. |
f0758ff
to
65a13ee
Compare
7c2aae3
to
3cd7652
Compare
3cd7652
to
35548cb
Compare
da6b0ad
to
58983c3
Compare
@fruffy I tried building using this branch on an aarch64 Ubuntu 24.04 system (not sure if those details are relevant yet -- also trying x86_64 Ubuntu 24.04 but not done yet), and I got this error when trying to use these steps to build p4c:
Here are the errors I saw near the end of the output of the failed
It appears that p4c requires CMAKE_BUILD_TYPE to be RELEASE or DEBUG, but Z3 requires it to be one of How did you get around that? |
We fixed it here: #4765 In your list of steps RELEASE is used. I actually am not sure whether the names are case-insensitive, Z3 only enforces the right naming. We could remove that check. Edit: Turns out this can matter in some cases: https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#case-sensitivity |
I will try "Release" instead of "RELEASE" and see if things go better. |
It isn't finished yet, but it is going much further when I use this command when building p4c:
instead of this command:
so as far as I can tell it is not case-insensitive. Well, or at least Z3's name check is not case-insensitive. |
58983c3
to
5c04437
Compare
Is there a reason you prefer this PR builds Z3 from source code, vs. getting precompiled binaries from the Z3 releases page? At least for versions since about Z3 4.12.x or so, it seems they release precompiled binaries for Linux for both x86_64 and arm64 processors. Not a big deal to me either way -- mainly curious. |
Yes, starting with 4.12 P4C will crash when used with the garbage collector BDWGC. The reason is that the Z3 developers added I can't figure out how to fix BDWGC and the only other way is to patch Z3. That is only possible when pulling and compiling from source. |
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 not read and thought about the code changes. However, I have checked out this branch on both an aarch64 and x86_64 Ubuntu 24.04 Linux VM, and built p4c from source using these changes, and they pass tests, and p4testgen basically works at generating test cases.
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 think the PR name is misleading. But the PR itself looks OK
c93d72d
to
3d82e37
Compare
Signed-off-by: fruffy <[email protected]>
Signed-off-by: fruffy <[email protected]> asdsa Signed-off-by: fruffy <[email protected]>
1a13c0b
to
63f323b
Compare
Fixes #4693.