[PATCH U-Boot] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

From: Brian Norris briannorris@chromium.org
[linux commit ff64dd4857303dd5550faed9fd598ac90f0f2238]
git-diff-index does not refresh the index for you, so using it for a "-dirty" check can give misleading results. Commit 6147b1cf19651 ("scripts/setlocalversion: git: Make -dirty check more robust") tried to fix this by switching to git-status, but it overlooked the fact that git-status also writes to the .git directory of the source tree, which is definitely not kosher for an out-of-tree (O=) build. That is getting reverted.
Fortunately, git-status now supports avoiding writing to the index via the --no-optional-locks flag, as of git 2.14. It still calculates an up-to-date index, but it avoids writing it out to the .git directory.
So, let's retry the solution from commit 6147b1cf19651 using this new flag first, and if it fails, we assume this is an older version of git and just use the old git-diff-index method.
It's hairy to get the 'grep -vq' (inverted matching) correct by stashing the output of git-status (you have to be careful about the difference betwen "empty stdin" and "blank line on stdin"), so just pipe the output directly to grep and use a regex that's good enough for both the git-status and git-diff-index version.
Cc: Christian Kujau lists@nerdbynature.de Cc: Guenter Roeck linux@roeck-us.net Suggested-by: Alexander Kapshuk alexander.kapshuk@gmail.com Signed-off-by: Brian Norris briannorris@chromium.org Tested-by: Genki Sky sky@genki.is Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- This fixes real problems when building U-Boot via Yocto, since Yocto creates hard-links to certain files in the source repository, causing the "git diff-index" method to report -dirty, even if no file contents are actually changed. See e.g.
https://lists.openembedded.org/g/openembedded-core/message/137702
scripts/setlocalversion | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/scripts/setlocalversion b/scripts/setlocalversion index 8564bedd1a..55f8ace2ee 100755 --- a/scripts/setlocalversion +++ b/scripts/setlocalversion @@ -72,8 +72,16 @@ scm_version() printf -- '-svn%s' "`git svn find-rev $head`" fi
- # Check for uncommitted changes - if git diff-index --name-only HEAD | grep -qv "^scripts/package"; then + # Check for uncommitted changes. + # First, with git-status, but --no-optional-locks is only + # supported in git >= 2.14, so fall back to git-diff-index if + # it fails. Note that git-diff-index does not refresh the + # index, so it may give misleading results. See + # git-update-index(1), git-diff-index(1), and git-status(1). + if { + git --no-optional-locks status -uno --porcelain 2>/dev/null || + git diff-index --name-only HEAD + } | grep -qvE '^(.. )?scripts/package'; then printf '%s' -dirty fi

On Tue, Aug 25, 2020 at 01:29:50PM +0200, Rasmus Villemoes wrote:
From: Brian Norris briannorris@chromium.org
[linux commit ff64dd4857303dd5550faed9fd598ac90f0f2238]
git-diff-index does not refresh the index for you, so using it for a "-dirty" check can give misleading results. Commit 6147b1cf19651 ("scripts/setlocalversion: git: Make -dirty check more robust") tried to fix this by switching to git-status, but it overlooked the fact that git-status also writes to the .git directory of the source tree, which is definitely not kosher for an out-of-tree (O=) build. That is getting reverted.
Fortunately, git-status now supports avoiding writing to the index via the --no-optional-locks flag, as of git 2.14. It still calculates an up-to-date index, but it avoids writing it out to the .git directory.
So, let's retry the solution from commit 6147b1cf19651 using this new flag first, and if it fails, we assume this is an older version of git and just use the old git-diff-index method.
It's hairy to get the 'grep -vq' (inverted matching) correct by stashing the output of git-status (you have to be careful about the difference betwen "empty stdin" and "blank line on stdin"), so just pipe the output directly to grep and use a regex that's good enough for both the git-status and git-diff-index version.
Cc: Christian Kujau lists@nerdbynature.de Cc: Guenter Roeck linux@roeck-us.net Suggested-by: Alexander Kapshuk alexander.kapshuk@gmail.com Signed-off-by: Brian Norris briannorris@chromium.org Tested-by: Genki Sky sky@genki.is Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
This fixes real problems when building U-Boot via Yocto, since Yocto creates hard-links to certain files in the source repository, causing the "git diff-index" method to report -dirty, even if no file contents are actually changed. See e.g.
https://lists.openembedded.org/g/openembedded-core/message/137702
scripts/setlocalversion | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
It looks like we have one local change to setlocalversion since our sync from v3.16. Can you please re-sync us to the v5.8 release? Thanks!

On 25/08/2020 14.56, Tom Rini wrote:
On Tue, Aug 25, 2020 at 01:29:50PM +0200, Rasmus Villemoes wrote:
From: Brian Norris briannorris@chromium.org
[linux commit ff64dd4857303dd5550faed9fd598ac90f0f2238]
scripts/setlocalversion | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
It looks like we have one local change to setlocalversion since our sync from v3.16. Can you please re-sync us to the v5.8 release? Thanks!
Hmm, I suppose I could, but it's not really clear whether we'd still need to apply that fix (81630a3b38c2, scripts: setlocalversion: safely extract variables from auto.conf using awk), or if that has been obsoleted by your a356e7a86b83 (spl: Kconfig: Escape '$(ARCH)' in LDSCRIPT entries).
Rasmus

On Tue, Aug 25, 2020 at 04:53:36PM +0200, Rasmus Villemoes wrote:
On 25/08/2020 14.56, Tom Rini wrote:
On Tue, Aug 25, 2020 at 01:29:50PM +0200, Rasmus Villemoes wrote:
From: Brian Norris briannorris@chromium.org
[linux commit ff64dd4857303dd5550faed9fd598ac90f0f2238]
scripts/setlocalversion | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
It looks like we have one local change to setlocalversion since our sync from v3.16. Can you please re-sync us to the v5.8 release? Thanks!
Hmm, I suppose I could, but it's not really clear whether we'd still need to apply that fix (81630a3b38c2, scripts: setlocalversion: safely extract variables from auto.conf using awk), or if that has been obsoleted by your a356e7a86b83 (spl: Kconfig: Escape '$(ARCH)' in LDSCRIPT entries).
Ah, it's entirely likely we could just use setlocalversion as-is.

The linux changes since v3.16 are
78283edf2c01 kbuild: setlocalversion: print error to STDERR b24413180f56 License cleanup: add SPDX GPL-2.0 license identifier to files with no license 6147b1cf1965 scripts/setlocalversion: git: Make -dirty check more robust 8ef14c2c41d9 Revert "scripts/setlocalversion: git: Make -dirty check more robust" ff64dd485730 scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks 7a82e3fa28f1 scripts/setlocalversion: clear local variable to make it work for sh 991b78fbd223 scripts: setlocalversion: fix a bashism 3c96bdd0ebfa scripts: setlocalversion: replace backquote to dollar parenthesis
and it's ff64dd485730 that is the motivation for this sync. It fixes false positive "-dirty" added to the version string when building with Yocto (https://lists.openembedded.org/g/openembedded-core/message/137702).
There has been one U-Boot specific patch since this was synced with linux 3.16: 81630a3b38c2 (scripts: setlocalversion: safely extract variables from auto.conf using awk). However, it seems that since a356e7a86b (spl: Kconfig: Escape '$(ARCH)' in LDSCRIPT entries), auto.conf doesn't contain the $(ARCH) string but instead the actual value of ARCH.
So for now, use the linux version as-is; it should be fairly obvious if 81630a3b38c2 or something like it does need to be applied on top.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- scripts/setlocalversion | 45 +++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/scripts/setlocalversion b/scripts/setlocalversion index 8564bedd1a..20f2efd57b 100755 --- a/scripts/setlocalversion +++ b/scripts/setlocalversion @@ -1,4 +1,5 @@ #!/bin/sh +# SPDX-License-Identifier: GPL-2.0 # # This scripts adds local version information from the version # control systems git, mercurial (hg) and subversion (svn). @@ -44,11 +45,11 @@ scm_version()
# Check for git and a git repo. if test -z "$(git rev-parse --show-cdup 2>/dev/null)" && - head=`git rev-parse --verify --short HEAD 2>/dev/null`; then + head=$(git rev-parse --verify --short HEAD 2>/dev/null); then
# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore # it, because this version is defined in the top level Makefile. - if [ -z "`git describe --exact-match 2>/dev/null`" ]; then + if [ -z "$(git describe --exact-match 2>/dev/null)" ]; then
# If only the short version is requested, don't bother # running further git commands @@ -58,7 +59,7 @@ scm_version() fi # If we are past a tagged commit (like # "v2.6.30-rc5-302-g72357d5"), we pretty print it. - if atag="`git describe 2>/dev/null`"; then + if atag="$(git describe 2>/dev/null)"; then echo "$atag" | awk -F- '{printf("-%05d-%s", $(NF-1),$(NF))}'
# If we don't have a tag at all we print -g{commitish}. @@ -69,11 +70,19 @@ scm_version()
# Is this git on svn? if git config --get svn-remote.svn.url >/dev/null; then - printf -- '-svn%s' "`git svn find-rev $head`" + printf -- '-svn%s' "$(git svn find-rev $head)" fi
- # Check for uncommitted changes - if git diff-index --name-only HEAD | grep -qv "^scripts/package"; then + # Check for uncommitted changes. + # First, with git-status, but --no-optional-locks is only + # supported in git >= 2.14, so fall back to git-diff-index if + # it fails. Note that git-diff-index does not refresh the + # index, so it may give misleading results. See + # git-update-index(1), git-diff-index(1), and git-status(1). + if { + git --no-optional-locks status -uno --porcelain 2>/dev/null || + git diff-index --name-only HEAD + } | grep -qvE '^(.. )?scripts/package'; then printf '%s' -dirty fi
@@ -82,15 +91,15 @@ scm_version() fi
# Check for mercurial and a mercurial repo. - if test -d .hg && hgid=`hg id 2>/dev/null`; then + if test -d .hg && hgid=$(hg id 2>/dev/null); then # Do we have an tagged version? If so, latesttagdistance == 1 - if [ "`hg log -r . --template '{latesttagdistance}'`" == "1" ]; then - id=`hg log -r . --template '{latesttag}'` + if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; then + id=$(hg log -r . --template '{latesttag}') printf '%s%s' -hg "$id" else - tag=`printf '%s' "$hgid" | cut -d' ' -f2` + tag=$(printf '%s' "$hgid" | cut -d' ' -f2) if [ -z "$tag" -o "$tag" = tip ]; then - id=`printf '%s' "$hgid" | sed 's/[+ ].*//'` + id=$(printf '%s' "$hgid" | sed 's/[+ ].*//') printf '%s%s' -hg "$id" fi fi @@ -106,8 +115,8 @@ scm_version() fi
# Check for svn and a svn repo. - if rev=`LANG= LC_ALL= LC_MESSAGES=C svn info 2>/dev/null | grep '^Last Changed Rev'`; then - rev=`echo $rev | awk '{print $NF}'` + if rev=$(LANG= LC_ALL= LC_MESSAGES=C svn info 2>/dev/null | grep '^Last Changed Rev'); then + rev=$(echo $rev | awk '{print $NF}') printf -- '-svn%s' "$rev"
# All done with svn @@ -117,7 +126,7 @@ scm_version()
collect_files() { - local file res + local file res=
for file; do case "$file" in @@ -141,13 +150,9 @@ if $scm_only; then fi
if test -e include/config/auto.conf; then - # We are interested only in CONFIG_LOCALVERSION and - # CONFIG_LOCALVERSION_AUTO, so extract these in a safe - # way (i.e. w/o sourcing auto.conf) - CONFIG_LOCALVERSION=`cat include/config/auto.conf | awk -F '=' '/^CONFIG_LOCALVERSION=/ {print $2}'` - CONFIG_LOCALVERSION_AUTO=`cat include/config/auto.conf | awk -F '=' '/^CONFIG_LOCALVERSION_AUTO=/ {print $2}'` + . include/config/auto.conf else - echo "Error: kernelrelease not valid - run 'make prepare' to update it" + echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2 exit 1 fi

On Tue, Aug 25, 2020 at 05:28:39PM +0200, Rasmus Villemoes wrote:
The linux changes since v3.16 are
78283edf2c01 kbuild: setlocalversion: print error to STDERR b24413180f56 License cleanup: add SPDX GPL-2.0 license identifier to files with no license 6147b1cf1965 scripts/setlocalversion: git: Make -dirty check more robust 8ef14c2c41d9 Revert "scripts/setlocalversion: git: Make -dirty check more robust" ff64dd485730 scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks 7a82e3fa28f1 scripts/setlocalversion: clear local variable to make it work for sh 991b78fbd223 scripts: setlocalversion: fix a bashism 3c96bdd0ebfa scripts: setlocalversion: replace backquote to dollar parenthesis
and it's ff64dd485730 that is the motivation for this sync. It fixes false positive "-dirty" added to the version string when building with Yocto (https://lists.openembedded.org/g/openembedded-core/message/137702).
There has been one U-Boot specific patch since this was synced with linux 3.16: 81630a3b38c2 (scripts: setlocalversion: safely extract variables from auto.conf using awk). Keep these changes applied.
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk [trini: Re-add 81630a3b38c2 and reword message] Signed-off-by: Tom Rini trini@konsulko.com
Build testing the world showed that we really did need to keep 81630a3b38c2 so I put that back in (fixing whitespace in it) and reworded the commit message.
Applied to u-boot/master, thanks!
participants (2)
-
Rasmus Villemoes
-
Tom Rini