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

[C++] C++ build sometimes changes .git ownership #37767

Closed
pitrou opened this issue Sep 18, 2023 · 7 comments · Fixed by #38003
Closed

[C++] C++ build sometimes changes .git ownership #37767

pitrou opened this issue Sep 18, 2023 · 7 comments · Fixed by #38003
Assignees
Milestone

Comments

@pitrou
Copy link
Member

pitrou commented Sep 18, 2023

Describe the bug, including details regarding any error messages, version, and platform.

When I run the conda-integration docker build, the C++ build step sometimes changes ownership of files in the .git directory (which later breaks git commands until I fix it with chown):

$ ls -la .git
total 1108
drwxrwxr-x   9 antoine antoine   4096 sept. 18 15:36 .
drwxrwxr-x  21 antoine antoine   4096 sept. 18 15:31 ..
-rw-rw-r--   1 antoine antoine     41 sept. 18 15:32 AUTO_MERGE
drwxrwxr-x   2 antoine antoine   4096 sept.  8  2021 branches
-rw-rw-r--   1 antoine antoine   1949 sept. 14 20:02 COMMIT_EDITMSG
-rw-rw-r--   1 antoine antoine   8262 sept. 13  2022 COMMIT_EDITMSG.save
-rw-rw-r--   1 antoine antoine  54132 sept. 14 20:02 config
-rw-rw-r--   1 antoine antoine     73 sept.  8  2021 description
-rw-r--r--   1 antoine antoine   2130 sept. 18 15:30 FETCH_HEAD
-rw-rw-r--   1 antoine antoine     43 sept. 18 15:32 HEAD
drwxrwxr-x   2 antoine antoine   4096 sept.  8  2021 hooks
-rw-r--r--   1 root    root    848484 sept. 18 15:36 index
drwxrwxr-x   2 antoine antoine   4096 août  21 19:05 info
drwxrwxr-x   3 antoine antoine   4096 août  21 19:04 logs
drwxrwxr-x   4 antoine antoine   4096 sept.  8  2021 modules
drwxrwxr-x 260 antoine antoine   4096 sept. 18 15:30 objects
-rw-rw-r--   1 antoine antoine     41 sept. 18 15:30 ORIG_HEAD
-rw-rw-r--   1 antoine antoine 148298 sept.  8 09:09 packed-refs
drwxrwxr-x   6 antoine antoine   4096 août  31 22:52 refs

It also doesn't happen everytime...

Component(s)

C++

@pitrou
Copy link
Member Author

pitrou commented Sep 18, 2023

@assignUser @kou Do you have an idea where it might come from?

@assignUser
Copy link
Member

IIRC this is due to the docker build running with root user... e.g. we have to sudo/chown outputfiles to bundle them in r-binary-packages. Why it would change something in .git I am not sure...

@pitrou
Copy link
Member Author

pitrou commented Sep 18, 2023

Yes, but this doesn't answer the question why. This seems to happen specifically during the CMake phase (either setup or build, I'm not sure).

@assignUser
Copy link
Member

assignUser commented Sep 18, 2023

Hm probably externalProject in thirdpartytoolchain? That would be cached until you clean the build tree. Unless something in the integration job does it... (Checking out arrow-rs?)

@pitrou
Copy link
Member Author

pitrou commented Sep 18, 2023

As I said, this happens specifically during the C++ cmake commands.

@kou
Copy link
Member

kou commented Sep 19, 2023

Interesting. We have two codes that call git explicitly:

if(NOT ARROW_GIT_ID)
execute_process(COMMAND "git" "log" "-n1" "--format=%H"
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
OUTPUT_VARIABLE ARROW_GIT_ID
OUTPUT_STRIP_TRAILING_WHITESPACE)
endif()
if(NOT ARROW_GIT_DESCRIPTION)
execute_process(COMMAND "git" "describe" "--tags" "--dirty"
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
ERROR_QUIET
OUTPUT_VARIABLE ARROW_GIT_DESCRIPTION
OUTPUT_STRIP_TRAILING_WHITESPACE)
endif()

I tried strace git ... and found that git log doesn't touch but git describe may touch .git/index.
Could you try archery docker run -eARROW_GIT_DESCRIPTION=apache-arrow-14.0.0.dev-XXX conda-integration?

@pitrou
Copy link
Member Author

pitrou commented Sep 19, 2023

Could you try archery docker run -eARROW_GIT_DESCRIPTION=apache-arrow-14.0.0.dev-XXX conda-integration?

That works, thank you.

kou added a commit to kou/arrow that referenced this issue Oct 4, 2023
We run "git describe --tag --dirty" implicitly in
cpp/cmake_modules/DefineOptions.cmake. If we use "--dirty",
.git/index's owner may be changed. Because "git describe" touches
.git/index for "--dirty".

We can avoid changing .git/index's owner by not using "--dirty".
assignUser pushed a commit that referenced this issue Oct 11, 2023
### Rationale for this change

We run "git describe --tag --dirty" implicitly in
cpp/cmake_modules/DefineOptions.cmake. If we use "--dirty", .git/index's owner may be changed. Because "git describe" touches .git/index for "--dirty".

We can avoid changing .git/index's owner by not using "--dirty".

### What changes are included in this PR?

Remove "--dirty".

### Are these changes tested?

Yes. I used "strace git describe ...".

### Are there any user-facing changes?

No.
* Closes: #37767

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@assignUser assignUser added this to the 14.0.0 milestone Oct 11, 2023
JerAguilon pushed a commit to JerAguilon/arrow that referenced this issue Oct 23, 2023
### Rationale for this change

We run "git describe --tag --dirty" implicitly in
cpp/cmake_modules/DefineOptions.cmake. If we use "--dirty", .git/index's owner may be changed. Because "git describe" touches .git/index for "--dirty".

We can avoid changing .git/index's owner by not using "--dirty".

### What changes are included in this PR?

Remove "--dirty".

### Are these changes tested?

Yes. I used "strace git describe ...".

### Are there any user-facing changes?

No.
* Closes: apache#37767

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
### Rationale for this change

We run "git describe --tag --dirty" implicitly in
cpp/cmake_modules/DefineOptions.cmake. If we use "--dirty", .git/index's owner may be changed. Because "git describe" touches .git/index for "--dirty".

We can avoid changing .git/index's owner by not using "--dirty".

### What changes are included in this PR?

Remove "--dirty".

### Are these changes tested?

Yes. I used "strace git describe ...".

### Are there any user-facing changes?

No.
* Closes: apache#37767

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
### Rationale for this change

We run "git describe --tag --dirty" implicitly in
cpp/cmake_modules/DefineOptions.cmake. If we use "--dirty", .git/index's owner may be changed. Because "git describe" touches .git/index for "--dirty".

We can avoid changing .git/index's owner by not using "--dirty".

### What changes are included in this PR?

Remove "--dirty".

### Are these changes tested?

Yes. I used "strace git describe ...".

### Are there any user-facing changes?

No.
* Closes: apache#37767

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants