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

CI: Stop installing cvc4 on archlinux #15085

Merged
merged 1 commit into from May 13, 2024
Merged

Conversation

blishko
Copy link
Contributor

@blishko blishko commented May 8, 2024

This change is in preparation for upgrading to cvc5. Since archlinux is not running SMT tests anyway, we can drop cvc4 from the build immediately.

@@ -1273,7 +1273,7 @@ jobs:
- run:
name: Install runtime dependencies
command: |
pacman --noconfirm -Syu --noprogressbar --needed base-devel boost cmake z3 cvc4 git openssh tar
pacman --noconfirm -Syu --noprogressbar --needed base-devel boost cmake z3 git openssh tar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually understand why z3 is installed for t_archlinux_soltest but not for b_archlinux.

Copy link
Member

@r0qs r0qs May 8, 2024

Choose a reason for hiding this comment

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

Yeah, I was about to comment about it. You can remove this installation step. It is already installed in b_archlinux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, everything except z3 😄

I am also wondering about this in b_archlinux

# This can be switched off if we run out of sync with Arch.
      USE_Z3: ON

Should we switch it off?

Copy link
Member

@r0qs r0qs May 8, 2024

Choose a reason for hiding this comment

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

No, keep it for now, and also installing z3 in that step. We actually need to think about if we will reenable the tests once we update the z3 version (i.e. removing the --no-smt flag). But for now let's keep that flag to disable the tests as well.

So just change the step to:

pacman --noconfirm -Syu --noprogressbar --needed z3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the step accordingly and the CI is still green.

cameel
cameel previously approved these changes May 10, 2024
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Well, if CI passes here, it proves that we're not really running anything requiring cvc4 on Arch Linux.

@blishko
Copy link
Contributor Author

blishko commented May 10, 2024

Before we merge this, I wanted to follow up on Rodrigo's suggestion to remove installation of dependencies in t_archlinux_soltest, because these are already installed in b_archlinux. Except for Z3. That is also weird, because my intuition is that Z3 should be installed in the b job, not the t job.

@cameel
Copy link
Member

cameel commented May 10, 2024

Ah, ok then. I wasn't going to merge it myself anyway. Just approve it and let you do it yourself in case you still wanted to change something :) Though if you're planning changes it's always safer to revert the PR back to draft.

Except for Z3. That is also weird, because my intuition is that Z3 should be installed in the b job, not the t job.

The binary is not completely static. Z3 is actually the one thing we link dynamically and the z3 package contains that dynamic library. You need it to be installed when you run solc.

EDIT: Actually, this job does not even build a static binary. But this does not change the situation with Z3 :)

This change is in preparation for upgrading to cvc5. Since archlinux is
not running SMT tests anyway, we can drop cvc4 from the build
immediately.
@blishko
Copy link
Contributor Author

blishko commented May 13, 2024

The binary is not completely static. Z3 is actually the one thing we link dynamically and the z3 package contains that dynamic library. You need it to be installed when you run solc.

@cameel, @r0qs, can you help me understand this?
During compilation, Z3 is not present, so Z3-related files are not compiled (e.g., Z3Interface.cpp, Z3CHCInterface.h).
Also, on archlinux, the cmake option USE_Z3_DLOPEN is OFF, so my conclusion is that installing Z3 for the testing job is useless, it does not do anything.

My opinion is that either Z3 should be installed for the build job already, or not at all. What am I missing?

@r0qs
Copy link
Member

r0qs commented May 13, 2024

The binary is not completely static. Z3 is actually the one thing we link dynamically and the z3 package contains that dynamic library. You need it to be installed when you run solc.

@cameel, @r0qs, can you help me understand this? During compilation, Z3 is not present, so Z3-related files are not compiled (e.g., Z3Interface.cpp, Z3CHCInterface.h). Also, on archlinux, the cmake option USE_Z3_DLOPEN is OFF, so my conclusion is that installing Z3 for the testing job is useless, it does not do anything.

Oh, I thought that USE_Z3_DLOPEN was ON, but you are right, it is OFF by default. So in this case it doesn't seem to make sense to install z3 indeed.

My opinion is that either Z3 should be installed for the build job already, or not at all. What am I missing?

Yeah, I think we should just remove z3 in this case then. Since we are running the tests with --no-smt. We could quickly discuss that during the call today. One of the problems that I remember of having z3 enable on the tests running on Archlinux is related to the version of z3, that was constantly mismatch due to the Archlinux rolling releases. So maybe, we could even consider re-enable such tests and add -DSTRICT_Z3_VERSION=OFF, so we would catch possible breaking changes on z3 library.

@blishko
Copy link
Contributor Author

blishko commented May 13, 2024

OK, how about we continue with the conversation about Z3 in our CI channel.
I think we can merge this PR as is now, no?

Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Well, there's still some open questions about how to deal with this stuff going forward - but not much harm in merging this for now.

@blishko blishko merged commit d116a34 into develop May 13, 2024
72 checks passed
@blishko blishko deleted the smt-drop-cvc4-archlinux branch May 13, 2024 12:47
@cameel
Copy link
Member

cameel commented May 13, 2024

The binary is not completely static. Z3 is actually the one thing we link dynamically and the z3 package contains that dynamic library. You need it to be installed when you run solc.

Also, on archlinux, the cmake option USE_Z3_DLOPEN is OFF, so my conclusion is that installing Z3 for the testing job is useless, it does not do anything.

My opinion is that either Z3 should be installed for the build job already, or not at all. What am I missing?

Ah, right. I assumed it was ON. I mean, my impression was that you were actually having some problems after removing z3 from the test job and asking why that is. But rereading now, I don't see any mention of that, you're just asking if it's ok to remove it. I'd say that if the job works without it then it's fine. I'd expect a crash/error otherwise. With a USE_Z3_DLOPEN=OFF build it indeed shouldn't be needed.

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

Successfully merging this pull request may close these issues.

None yet

4 participants