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

Docker: Fix git perms issue during copter build #28427

Merged

Conversation

henrywarhurst
Copy link
Contributor

I encountered the follow issue during the docker build:

ardupilot@35af3dd2b3ee:/ardupilot$ ./waf copter
Waf: Entering directory `/ardupilot/build/sitl'
Embedding file locations.txt:Tools/autotest/locations.txt
Embedding file models/Callisto.json:Tools/autotest/models/Callisto.json
Embedding file models/freestyle.json:Tools/autotest/models/freestyle.json
Embedding file models/plane-3d.parm:Tools/autotest/models/plane-3d.parm
Embedding file models/plane.parm:Tools/autotest/models/plane.parm
Embedding file models/xplane_heli.json:Tools/autotest/models/xplane_heli.json
Embedding file models/xplane_plane.json:Tools/autotest/models/xplane_plane.json
Command ['/usr/bin/git', 'rev-parse', '--short=8', 'HEAD'] returned 128

My system:

  • OSX 15.0.1 (24A348)
  • Docker Desktop 4.33.0 (160616)
  • Fresh clone of ardupilot 145cc4b

@henrywarhurst henrywarhurst changed the title Fix git perms issue during docker copter build Docker: Fix git perms issue during docker copter build Oct 17, 2024
@henrywarhurst henrywarhurst changed the title Docker: Fix git perms issue during docker copter build Docker: Fix git perms issue during copter build Oct 17, 2024
@henrywarhurst
Copy link
Contributor Author

@rmackay9 maybe could you have a look at this one

@khancyr
Copy link
Contributor

khancyr commented Oct 18, 2024

@henrywarhurst shouldn't you do this on host ?

@khancyr khancyr added the DevEnv label Oct 18, 2024
@henrywarhurst
Copy link
Contributor Author

@khancyr maybe I'm not sure what you mean, but the problem seems localized to inside the container. Open to other ideas, but git doesn't seem broken on the host side, all the usual git commands work on host side without issue, and not in the container. Lmk though! Maybe I'm misunderstanding your point

@henrywarhurst
Copy link
Contributor Author

When I run git status inside the container, this happens (before my change):

ardupilot@35af3dd2b3ee:/ardupilot$ git status
fatal: detected dubious ownership in repository at '/ardupilot'
To add an exception for this directory, call:

	git config --global --add safe.directory /ardupilot

Outside the container on the host, git status has no issues

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

ok, could you just move it before L36 where we call install-prereqs-ubuntu.sh ? there is a git call in it

and it should be good.
I assume that is some macos permission thingsbut it shouldn't bother on linux side

@henrywarhurst
Copy link
Contributor Author

henrywarhurst commented Oct 18, 2024

done @khancyr. Yeah probably an OSX specific issue

EDIT:
Actually I can't put it before L36 because git isn't installed yet then. Put it just after and added a comment

@henrywarhurst henrywarhurst force-pushed the hwarhurst/fix-docker-git branch 2 times, most recently from 6d41fbd to 581eb18 Compare October 18, 2024 08:54
@henrywarhurst
Copy link
Contributor Author

@khancyr would you mind having another look and approving if this looks good?

@henrywarhurst
Copy link
Contributor Author

friendly ping @khancyr

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

Looks good for the code.

For the commit style : you need to rebase to get ride of the merge commit.
And start the commit msg with "Docker:" so we can track it is docker change ! And it should be good to merge

@henrywarhurst henrywarhurst force-pushed the hwarhurst/fix-docker-git branch 2 times, most recently from 1308488 to e58a78c Compare October 25, 2024 19:39
@henrywarhurst
Copy link
Contributor Author

Thanks @khancyr. I cleaned up the commit history

@henrywarhurst henrywarhurst force-pushed the hwarhurst/fix-docker-git branch 3 times, most recently from e74f21f to 6e066c5 Compare October 27, 2024 14:00
Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

LGTM

@henrywarhurst
Copy link
Contributor Author

thanks @khancyr. Any idea why this isn't merging on its own?

@khancyr
Copy link
Contributor

khancyr commented Nov 1, 2024

@henrywarhurst we don't have automerge so a maintainer need to manual action to merge it

@peterbarker peterbarker merged commit 4c9da02 into ArduPilot:master Nov 3, 2024
5 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks!

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.

3 participants