Update docker z3 version 4.8.16 #12967
Conversation
leonardoalt
force-pushed
the
update_z3_docker
branch
from
0fbb0d5 to
7bacf8e
Compare
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Tests are failing here. |
Yea will rebase it now that the tests were merged |
leonardoalt
force-pushed
the
update_z3_docker
branch
2 times, most recently
from
31d9ea3 to
90e2ca9
Compare
|
@hrkrshnn this can now be merged too |
scripts/ci/build.sh
Outdated
| @@ -4,6 4,10 @@ set -ex | |||
| ROOTDIR="$(dirname "$0")/../.." | |||
| cd "${ROOTDIR}" | |||
|
|
|||
| ABSOLUTE_PATH=$(pwd) | |||
|
|
|||
| git config --global --add safe.directory "${ABSOLUTE_PATH}" | |||
There was a problem hiding this comment.
Interesting. Looking at the documentation: https://git-scm.com/docs/git-config#Documentation/git-config.txt-safedirectory
This feels a bit scary. This is where the release binaries are built right? I wonder if a git hook can influence the binary somehow?
There was a problem hiding this comment.
Also, the --global flag modifies user's ~/.gitconfig. I think it would be better to modify config only for the current repo in case someone has to run this script on his machine.
There was a problem hiding this comment.
I just saw this in the docs:
This config setting is only respected when specified in a system or global config, not when it is specified in a repository config or via the command line option
So I see why it's --global. But in that case I think this is something that should be done in the docker image, not in the build script.
There was a problem hiding this comment.
This feels a bit scary. This is where the release binaries are built right? I wonder if a git hook can influence the binary somehow?
After taking a closer look at it, the root of the issue is that we're giving git inside the container a repo dir owned by the user from outside of the container. git notices different users and bails. I don't think there's any inherent risk in that. At least it's not that different from the situation where you just clone the repo and run git on it. It's really the same user doing things.
The thing that's kinda bad is that we're running as root inside the container and it's not a good practice but this is independent of this PR. We already have this problem.
Overall we could work around the issue by chowning the files or by ensuring that we use the same user on both sides. But I think that safe.directory fix is good enough. Just IMO the build.sh script is not the best place to run it since it's a global change. I think it belongs either in the dockerfile or in docker_upgrade.sh. Probably the latter since the repo dir is not hard-coded in the image and it's the script that attaches the volume that determines that location.
|
I have just pushed a slightly different workaround for the issue. I moved the git config change to I also found the answer for why this is needed: https://github.blog/2022-04-12-git-security-vulnerability-announced/ |
|
|
|
|
|
|
|
|
|
So we should change the docker hashes because the other PR was already merged. |
|
Yeah, but that's after we merge this right? BTW, I just rebased it on develop to make the failing archlinux soltest job pass. So we'll get new hashes in a moment. |
Shouldn't it be the same hashes as the last ones posted? |
|
The docker images are already published anyway, whether we merge this or not. So we'd usually create another PR using the hashes here, and when everything works there, we merge both of them. |
|
|
|
|
|
|
|
|
Would be nice if they were but the build process is not that deterministic unfortunately.
OK, we can do it that way too. I guess this way we'll know for sure that CI passes with these images. |
Yea exactly |
|
@leo Wait, I just noticed that the hashes that are in I'm not sure if I should now just update the hashes without updating versions or bump the versions and rebuild again? |
|
Wrong username, I think? |
haha yes |
The other PR, the one that uses the hashes from here, is usually merged first. It was merged assuming this one was fine |
|
@cameel let's bump the versions in the dockerfiles and get new hashes just to be sure they're updated |
|
Unfortunately looks like I can't bump versions because |
No description provided.