| Message ID | 20220811134533.508348-1-john@metanate.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Yann E. MORIN |
| Headers | show |
| Series | support/download/git: disable global & system config | expand |
John, All, On 2022-08-11 14:45 +0100, John Keeping spake thusly: > The build environment should be isolated from the host system as much as > possible to keep the build reproducible. Git's global config (usually > ~/.gitconfig) and system config (/etc/gitconfig) can affect the > behaviour of all Git operations, so should be disabled. > > An example of this is that `git lfs install` will add the LFS smudge > filter to the global config and thus always checkout LFS files ignoring > the value of $(PKG)_GIT_LFS. This may mask a bug in the package when > the initial developer's machine has LFS installed globally. While I appreciate the reasoning and example, there are valid cases where we do want to use (at least) the user's settings, like when they have proxy commands (i.e. core.gitProxy or core.sshCommand) set to reach the outter world (e.g. because they are behind a restrictive firewall and need to tunnel ssh-over-https for example). So we need something better than just ignoring the local configuration, and this is going to be... tricky. Regards, Yann E. MORIN. > Signed-off-by: John Keeping <john@metanate.com> > --- > support/download/git | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/support/download/git b/support/download/git > index 1a1c315f73..b0a677c3a3 100755 > --- a/support/download/git > +++ b/support/download/git > @@ -82,13 +82,17 @@ trap _on_error ERR > # being expanded a second time (in case there are spaces in them) > _git() { > if [ -z "${quiet}" ]; then > - printf '%s ' GIT_DIR="${git_cache}/.git" ${GIT} "${@}"; printf '\n' > + printf '%s ' GIT_CONFIG_GLOBAL=/dev/null \ > + GIT_CONFIG_SYSTEM=/dev/null \ > + GIT_DIR="${git_cache}/.git" ${GIT} "${@}" > + printf '\n' > fi > _plain_git "$@" > } > # Note: please keep command below aligned with what is printed above > _plain_git() { > - eval GIT_DIR="${git_cache}/.git" ${GIT} "${@}" > + eval GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ > + GIT_DIR="${git_cache}/.git" ${GIT} "${@}" > } > > # Create a warning file, that the user should not use the git cache. > -- > 2.37.1 >
Hi Yann, On Sun, Aug 14, 2022 at 12:11:43AM +0200, Yann E. MORIN wrote: > On 2022-08-11 14:45 +0100, John Keeping spake thusly: > > The build environment should be isolated from the host system as much as > > possible to keep the build reproducible. Git's global config (usually > > ~/.gitconfig) and system config (/etc/gitconfig) can affect the > > behaviour of all Git operations, so should be disabled. > > > > An example of this is that `git lfs install` will add the LFS smudge > > filter to the global config and thus always checkout LFS files ignoring > > the value of $(PKG)_GIT_LFS. This may mask a bug in the package when > > the initial developer's machine has LFS installed globally. > > While I appreciate the reasoning and example, there are valid cases > where we do want to use (at least) the user's settings, like when they > have proxy commands (i.e. core.gitProxy or core.sshCommand) set to reach > the outter world (e.g. because they are behind a restrictive firewall > and need to tunnel ssh-over-https for example). Good point - this is more complicated than I initially realised. > So we need something better than just ignoring the local configuration, > and this is going to be... tricky. What do you thing about adding a config option to specify the global gitconfig file to use? Unfortunately we can't just default the value to ~/.gitconfig as there is no value which replicates the behaviour when the variable is unset (when Git uses both ~/.gitconfig and $XDG_CONFIG_HOME/git/config). The system config is less likely to be problematic as I don't think it's used much outside managed environments where the content probably is for proxy settings that should be included in Buildroot. So we end up with something like this: eval ${BR2_GIT_CONFIG_GLOBAL:+GIT_CONFIG_GLOBAL=\'${BR2_GIT_CONFIG_GLOBAL}\'} \ GIT_DIR="${git_cache}/.git" ${GIT} "${@}"
John, All, On 2022-08-15 11:46 +0100, John Keeping spake thusly: > On Sun, Aug 14, 2022 at 12:11:43AM +0200, Yann E. MORIN wrote: > > On 2022-08-11 14:45 +0100, John Keeping spake thusly: > > > The build environment should be isolated from the host system as much as > > > possible to keep the build reproducible. Git's global config (usually > > > ~/.gitconfig) and system config (/etc/gitconfig) can affect the > > > behaviour of all Git operations, so should be disabled. > > While I appreciate the reasoning and example, there are valid cases > > where we do want to use (at least) the user's settings, [...] > What do you thing about adding a config option to specify the global > gitconfig file to use? [--SNIP--] > eval ${BR2_GIT_CONFIG_GLOBAL:+GIT_CONFIG_GLOBAL=\'${BR2_GIT_CONFIG_GLOBAL}\'} \ > GIT_DIR="${git_cache}/.git" ${GIT} "${@}" Honestly, I don't think the issue warrants extra complexity. For your git-lfs example, we would notice a missing FO_GIT_LFS fairly quickly, thanks to the autobuilders. So this is not a big issue for packages in upstream Buildroot; for br2-external trees, this is most probably not an issue either: either all developpers ona project have the same settings, or they are using a sane build environment, or they would also notice, so this is not very important. Hwever, there are settings that we would probably want to always disable, like line-ending mangling (core.autocrlf or gitattributes) or other keyword replacement (gitattributes 'ident' or 'filter'...). But then, we haven't had much report about such failures, if at all, so this does not look like a problem in practice. So, I still don't think we need to add complexity to solve this issue. If we can get something really simple, that's OK, but no nuclear powerplant please. ;-) > The system config is less likely to be problematic as I don't think it's > used much outside managed environments where the content probably is for > proxy settings that should be included in Buildroot. Yes, I agree that system-wide config file should be Mostly Harmless (C). Regards, Yann E. MORIN.
On Mon, Aug 15, 2022 at 01:41:43PM +0200, Yann E. MORIN wrote: > On 2022-08-15 11:46 +0100, John Keeping spake thusly: > > On Sun, Aug 14, 2022 at 12:11:43AM +0200, Yann E. MORIN wrote: > > > On 2022-08-11 14:45 +0100, John Keeping spake thusly: > > > > The build environment should be isolated from the host system as much as > > > > possible to keep the build reproducible. Git's global config (usually > > > > ~/.gitconfig) and system config (/etc/gitconfig) can affect the > > > > behaviour of all Git operations, so should be disabled. > > > While I appreciate the reasoning and example, there are valid cases > > > where we do want to use (at least) the user's settings, [...] > > What do you thing about adding a config option to specify the global > > gitconfig file to use? > [--SNIP--] > > eval ${BR2_GIT_CONFIG_GLOBAL:+GIT_CONFIG_GLOBAL=\'${BR2_GIT_CONFIG_GLOBAL}\'} \ > > GIT_DIR="${git_cache}/.git" ${GIT} "${@}" > > Honestly, I don't think the issue warrants extra complexity. > > For your git-lfs example, we would notice a missing FO_GIT_LFS fairly > quickly, thanks to the autobuilders. So this is not a big issue for > packages in upstream Buildroot; for br2-external trees, this is most > probably not an issue either: either all developpers ona project have > the same settings, or they are using a sane build environment, or they > would also notice, so this is not very important. > > Hwever, there are settings that we would probably want to always > disable, like line-ending mangling (core.autocrlf or gitattributes) or > other keyword replacement (gitattributes 'ident' or 'filter'...). But > then, we haven't had much report about such failures, if at all, so this > does not look like a problem in practice. > > So, I still don't think we need to add complexity to solve this issue. > If we can get something really simple, that's OK, but no nuclear > powerplant please. ;-) I think the above is as simple as it gets, the only alternative I can see is explicit listing either permitted or banned config variables. But it turns out that $GIT_CONFIG_GLOBAL was only added last year (in Git v2.32) so it's not available everywhere yet and we can't rely on the installed Git supporting this. I'm dropping this now because it looks like the complexity is going to outweigh the benefit for any approach that doesn't use $GIT_CONFIG_GLOBAL.
diff --git a/support/download/git b/support/download/git index 1a1c315f73..b0a677c3a3 100755 --- a/support/download/git +++ b/support/download/git @@ -82,13 +82,17 @@ trap _on_error ERR # being expanded a second time (in case there are spaces in them) _git() { if [ -z "${quiet}" ]; then - printf '%s ' GIT_DIR="${git_cache}/.git" ${GIT} "${@}"; printf '\n' + printf '%s ' GIT_CONFIG_GLOBAL=/dev/null \ + GIT_CONFIG_SYSTEM=/dev/null \ + GIT_DIR="${git_cache}/.git" ${GIT} "${@}" + printf '\n' fi _plain_git "$@" } # Note: please keep command below aligned with what is printed above _plain_git() { - eval GIT_DIR="${git_cache}/.git" ${GIT} "${@}" + eval GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ + GIT_DIR="${git_cache}/.git" ${GIT} "${@}" } # Create a warning file, that the user should not use the git cache.
The build environment should be isolated from the host system as much as possible to keep the build reproducible. Git's global config (usually ~/.gitconfig) and system config (/etc/gitconfig) can affect the behaviour of all Git operations, so should be disabled. An example of this is that `git lfs install` will add the LFS smudge filter to the global config and thus always checkout LFS files ignoring the value of $(PKG)_GIT_LFS. This may mask a bug in the package when the initial developer's machine has LFS installed globally. Signed-off-by: John Keeping <john@metanate.com> --- support/download/git | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)