diff mbox series

[v3,1/2] scripts/setlocalversion: allow running in a subdir

Message ID 20201208200508.4107399-2-willmcvicker@google.com (mailing list archive)
State New, archived
Headers show
Series modules: add scmversion field | expand

Commit Message

William McVicker Dec. 8, 2020, 8:05 p.m. UTC
Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Jessica Yu Dec. 11, 2020, 3:33 p.m. UTC | #1
Hi Will,

+++ Will McVicker [08/12/20 20:05 +0000]:
>Getting the scmversion using scripts/setlocalversion currently only
>works when run at the root of a git or mecurial project. This was
>introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
>the linux source tree") so that if one is building within a subdir of
>a git tree that isn't the kernel git project, then the vermagic wouldn't
>include that git sha1. However, the proper solution to that is to just
>set this config in your defconfig:
>
>  # CONFIG_LOCALVERSION_AUTO is not set
>
>which is already the default in many defconfigs:
>
>  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
>  89
>
>So let's bring back this functionality so that we can use
>scripts/setlocalversion to capture the SCM version of external modules
>that reside within subdirectories of an SCM project.

Hm, this seems to essentially be a revert of commit 8558f59edf93.
AFAICT from light testing it also reintroduces the issue it was
originally trying to fix, no?

From the reporter:

    Dan McGee <dpmcgee@gmail.com> writes:
    > Note that when in git, you get the appended "+" sign. If
    > LOCALVERSION_AUTO is set, you will get something like
    > "eee-gb01b08c-dirty" (whereas the copy of the tree in /tmp still
    > returns "eee"). It doesn't matter whether the working tree is dirty or
    > clean.
    >
    > Is there a way to disable this? I'm building from a clean tarball that
    > just happens to be unpacked inside a git repository. One would think
    > setting LOCALVERSION_AUTO to false would do it, but no such luck...

Correct me if I'm wrong, but what I'm understanding is that the
original reporter was having trouble with setlocalversion appending
unwanted strings ("+" or "gXXXXXXX-dirty" etc) when building from a
clean tarball that happens to live inside a git repo. Even if
LOCALVERSION_AUTO is disabled it still appends the "+" string if the
SCM above the linux source tree is not at an annotated tag. Since
setlocalversion is getting confused by the presence of a different scm
that commit fixed this by confining the checks to the root of the
(possibly git managed) kernel source tree. Masahiro can probably
better comment since he maintains scripts/*.

In any case, this patch isn't of interest to in-tree modules, since we
can generate the scmversion perfectly fine without it, so I doubt it's
going to get any support here. Would you be fine with dropping the
first patch or would that pose issues?

>Signed-off-by: Will McVicker <willmcvicker@google.com>
>---
> scripts/setlocalversion | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
>diff --git a/scripts/setlocalversion b/scripts/setlocalversion
>index bb709eda96cd..cd42009e675b 100755
>--- a/scripts/setlocalversion
>+++ b/scripts/setlocalversion
>@@ -44,8 +44,7 @@ scm_version()
> 	fi
>
> 	# Check for git and a git repo.
>-	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
>-	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
>+	if head=$(git rev-parse --verify 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.
>@@ -102,7 +101,7 @@ scm_version()
> 	fi
>
> 	# Check for mercurial and a mercurial repo.
>-	if test -d .hg && hgid=$(hg id 2>/dev/null); then
>+	if 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}')
>-- 
>2.29.2.576.ga3fc446d84-goog
>
William McVicker Dec. 16, 2020, 10:08 p.m. UTC | #2
On Fri, Dec 11, 2020 at 04:33:59PM +0100, Jessica Yu wrote:
> Hi Will,
> 
> +++ Will McVicker [08/12/20 20:05 +0000]:
> > Getting the scmversion using scripts/setlocalversion currently only
> > works when run at the root of a git or mecurial project. This was
> > introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
> > the linux source tree") so that if one is building within a subdir of
> > a git tree that isn't the kernel git project, then the vermagic wouldn't
> > include that git sha1. However, the proper solution to that is to just
> > set this config in your defconfig:
> > 
> >  # CONFIG_LOCALVERSION_AUTO is not set
> > 
> > which is already the default in many defconfigs:
> > 
> >  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
> >  89
> > 
> > So let's bring back this functionality so that we can use
> > scripts/setlocalversion to capture the SCM version of external modules
> > that reside within subdirectories of an SCM project.
> 
> Hm, this seems to essentially be a revert of commit 8558f59edf93.
> AFAICT from light testing it also reintroduces the issue it was
> originally trying to fix, no?
> 
> From the reporter:
> 
>    Dan McGee <dpmcgee@gmail.com> writes:
>    > Note that when in git, you get the appended "+" sign. If
>    > LOCALVERSION_AUTO is set, you will get something like
>    > "eee-gb01b08c-dirty" (whereas the copy of the tree in /tmp still
>    > returns "eee"). It doesn't matter whether the working tree is dirty or
>    > clean.
>    >
>    > Is there a way to disable this? I'm building from a clean tarball that
>    > just happens to be unpacked inside a git repository. One would think
>    > setting LOCALVERSION_AUTO to false would do it, but no such luck...
> 
> Correct me if I'm wrong, but what I'm understanding is that the
> original reporter was having trouble with setlocalversion appending
> unwanted strings ("+" or "gXXXXXXX-dirty" etc) when building from a
> clean tarball that happens to live inside a git repo. Even if
> LOCALVERSION_AUTO is disabled it still appends the "+" string if the
> SCM above the linux source tree is not at an annotated tag. Since
> setlocalversion is getting confused by the presence of a different scm
> that commit fixed this by confining the checks to the root of the
> (possibly git managed) kernel source tree. Masahiro can probably
> better comment since he maintains scripts/*.
> 
> In any case, this patch isn't of interest to in-tree modules, since we
> can generate the scmversion perfectly fine without it, so I doubt it's
> going to get any support here. Would you be fine with dropping the
> first patch or would that pose issues?
> 
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> > scripts/setlocalversion | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > index bb709eda96cd..cd42009e675b 100755
> > --- a/scripts/setlocalversion
> > +++ b/scripts/setlocalversion
> > @@ -44,8 +44,7 @@ scm_version()
> > 	fi
> > 
> > 	# Check for git and a git repo.
> > -	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
> > -	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
> > +	if head=$(git rev-parse --verify 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.
> > @@ -102,7 +101,7 @@ scm_version()
> > 	fi
> > 
> > 	# Check for mercurial and a mercurial repo.
> > -	if test -d .hg && hgid=$(hg id 2>/dev/null); then
> > +	if 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}')
> > -- 
> > 2.29.2.576.ga3fc446d84-goog
> > 
> 
> -- 
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> 

Hi Jessica,

I'm okay with dropped this first patch since it does only related to external
modules. I'll upload v4 now with just the bits that related to in-tree modules.

Thanks,
Will
diff mbox series

Patch

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@  scm_version()
 	fi
 
 	# Check for git and a git repo.
-	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
+	if head=$(git rev-parse --verify 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.
@@ -102,7 +101,7 @@  scm_version()
 	fi
 
 	# Check for mercurial and a mercurial repo.
-	if test -d .hg && hgid=$(hg id 2>/dev/null); then
+	if 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}')