-
Notifications
You must be signed in to change notification settings - Fork 12
Docker fixes #38
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 fixes #38
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ services: | |
| image: gnuoctave/octave:latest | ||
| working_dir: /workspace | ||
| user: "${HOST_UID:?set HOST_UID or run via make check-local, not docker compose directly}:${HOST_GID:?set HOST_GID or run via make check-local, not docker compose directly}" | ||
| environment: | ||
| HOME: /workspace | ||
|
Comment on lines
+6
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works and makes sense, but it's also worth considering using I'm not 100% sure of any trade-offs but it's worth considering I think. |
||
| volumes: | ||
| - .:/workspace:z | ||
| command: make check-ci | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I was trying to be clever here and use this whole
GIT_CONFIG_COUNTmess to get around the fact that the tarball builds withgit archiveisn't run as the same user that owns the git repo (b/c it's run in docker) (after a deep rabbit hole that's thanks to CVE-2022-24765).But it turns out that feature was added in git 2.31, and guess what versions 6.4.0 and 7.3.0 got built with:
I think your solution is probably cleaner/simpler though I'm surprised it works with
safe.directoryb/c that got added later? But I'm guessing it was back-ported to previous releases specifically for the CVE. At any rate, if it works it works.