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

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

[Rasmus Villemoes]

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
 
-- 
2.23.0

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

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200825/f5ea5eb2/attachment.sig>

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

[Rasmus Villemoes]

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

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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200825/800a8cee/attachment.sig>

[PATCH v2] scripts/setlocalversion: sync with linux 5.8

[Rasmus Villemoes]

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
 
-- 
2.23.0

[PATCH v2] scripts/setlocalversion: sync with linux 5.8

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200828/02725995/attachment.sig>