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

Add submodule path to git safe.directory before initialize the submodule #15554

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Oct 30, 2024

We recently encountered errors while updating Z3 and CVC5 in the buildpack images for #15551.

See the build logs here: https://github.com/ethereum/solidity/actions/runs/11575096134/job/32220871629?pr=15551 and https://github.com/ethereum/solidity/actions/runs/11575096134/job/32220870627?pr=15551

 -- git submodule 'fmtlib' seem not to be initialized: implicitly executing 'git submodule update --init '/root/project/deps/fmtlib'.
-- Found Git: /usr/bin/git (found version "2.25.1") 
Submodule 'deps/fmtlib' (https://github.com/fmtlib/fmt.git) registered for path 'deps/fmtlib'
Cloning into '/root/project/deps/fmtlib'...
fatal: detected dubious ownership in repository at '/root/project/deps/fmtlib'
To add an exception for this directory, call:

	git config --global --add safe.directory /root/project/deps/fmtlib
Unable to fetch in submodule path 'deps/fmtlib'; trying to directly fetch 0c9fce2ffefecfdce794e1859584e25877b7b592:
fatal: detected dubious ownership in repository at '/root/project/deps/fmtlib'
To add an exception for this directory, call:

	git config --global --add safe.directory /root/project/deps/fmtlib
fatal: detected dubious ownership in repository at '/root/project/deps/fmtlib'
To add an exception for this directory, call:

	git config --global --add safe.directory /root/project/deps/fmtlib
Fetched in submodule path 'deps/fmtlib', but it did not contain 0c9fce2ffefecfdce794e1859584e25877b7b592. Direct fetching of that commit failed.
CMake Error at cmake/submodules.cmake:19 (message):
  Failed to initialize submodules: 'git submodule update --init' failed.
Call Stack (most recent call first):
  cmake/fmtlib.cmake:2 (initialize_submodule)
  CMakeLists.txt:61 (include)

This PR aims to address these issues by temporarily marking the submodules directory as a safe.directory before attempting to initialize it.

execute_process(
COMMAND git config --global --unset safe.directory ${CMAKE_SOURCE_DIR}/deps/${SUBMODULE_PATH}
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
ERROR_QUIET
Copy link
Member Author

Choose a reason for hiding this comment

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

Note I used ERROR_QUIET here to suppress errors in case the submodule's safe.directory entry doesn’t exist at the time of cleanup.

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.

I don't think that putting any commands modifying global config in our CMake files is a good idea, even if the change is temporary.

This might be acceptable (but still a hack) if limited to Dockerfiles or CI, but definitely not in the general build system that everyone runs.

@r0qs
Copy link
Member Author

r0qs commented Oct 30, 2024

I don't think that putting any commands modifying global config in our CMake files is a good idea, even if the change is temporary.

This might be acceptable (but still a hack) if limited to Dockerfiles or CI, but definitely not in the general build system that everyone runs.

Unfortunately, the safe.directory configuration is only available globally and cannot be applied locally, which is why I did the change in this way since now our submodule dependencies are managed by CMake, do you have any suggestions for a better solution?

@blishko
Copy link
Contributor

blishko commented Oct 30, 2024

There seems to be a difference in behaviour across different git versions.
The builds are failing for git 2.25.1, but are succeeding for git 2.43.0.

@r0qs
Copy link
Member Author

r0qs commented Oct 30, 2024

There seems to be a difference in behaviour across different git versions. The builds are failing for git 2.25.1, but are succeeding for git 2.43.0.

Indeed. Just found this here: https://github.blog/open-source/git/highlights-from-git-2-36/#stricter-repository-ownership-checks

Beginning in Git 2.35.2, Git changed its default behavior to prevent you from executing git commands
in a repository owned by a different user than the current one. This is designed to prevent git invocations
from unintentionally executing commands which the repository owner configured.

@r0qs
Copy link
Member Author

r0qs commented Oct 30, 2024

I don't think that putting any commands modifying global config in our CMake files is a good idea, even if the change is temporary.

This might be acceptable (but still a hack) if limited to Dockerfiles or CI, but definitely not in the general build system that everyone runs.

Maybe we could apply such configs in the build.sh script for each submodule before calling cmake. I believe those scripts will only be used by CI anyway. Would that be better?

@blishko
Copy link
Contributor

blishko commented Oct 30, 2024

Another way would be just to ensure to use the same user when creating the docker image and when running the build insider the container, no?

@ekpyron
Copy link
Member

ekpyron commented Oct 30, 2024

Why not just do the git config in the Dockerfile? Otherwise: yes, adjusting things, s.t. the user/permissions just match would be the most proper solution I guess. (And I agree that we definitely shouldn't do any of this in "user"-facing cmake config or scripts)

@r0qs r0qs marked this pull request as draft October 30, 2024 13:58
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system 🏗️ stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants