[v1] setlocalversion: use ${objtree}/include/config/auto.conf

[PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by HONG Yifan 9 months, 1 week ago

setlocalversion reads include/config/auto.conf, which is located below
$(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
some in-kernel build artifacts").

Hence, the setlocalversion script needs to use
$(objtree)/include/config/auto.conf as well.

Note that $(objtree) is not necessarily `.` when O (aka KBUILD_OUTPUT)
is set, because of commit 13b25489b6f8 ("kbuild: change working
directory to external module directory with M=").

Signed-off-by: HONG Yifan <elsk@google.com>
---
Implementation note: Should I test -z ${objtree} before using it? Otherwise it
looks at /include/config/auto.conf which is wrong.
Or should I fall back to `.` if objtree is not found in the environment
variables?
I could also `exit 1` if $objtree is not set. Please let me know what you think.

 scripts/setlocalversion | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 28169d7e143b..88f54eb5a7c2 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -186,7 +186,7 @@ if ${no_local}; then
 	exit 0
 fi

-if ! test -e include/config/auto.conf; then
+if ! test -e ${objtree}/include/config/auto.conf; then
 	echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
 	exit 1
 fi
--
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by Masahiro Yamada 9 months, 1 week ago

On Wed, Mar 12, 2025 at 11:12 AM HONG Yifan <elsk@google.com> wrote:
>
> setlocalversion reads include/config/auto.conf, which is located below
> $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> some in-kernel build artifacts").
>
> Hence, the setlocalversion script needs to use
> $(objtree)/include/config/auto.conf as well.
>
> Note that $(objtree) is not necessarily `.` when O (aka KBUILD_OUTPUT)
> is set, because of commit 13b25489b6f8 ("kbuild: change working
> directory to external module directory with M=").

Is this a real issue?
If so, please attach some commands to reproduce an issue.

setlocalversion is invoked only at line 1238 of the top-level Makefile,
within the check "ifeq ($(KBUILD_EXTMOD),)"
So, it is never called with external module builds.







> Signed-off-by: HONG Yifan <elsk@google.com>
> ---
> Implementation note: Should I test -z ${objtree} before using it? Otherwise it
> looks at /include/config/auto.conf which is wrong.
> Or should I fall back to `.` if objtree is not found in the environment
> variables?
> I could also `exit 1` if $objtree is not set. Please let me know what you think.
>
>  scripts/setlocalversion | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 28169d7e143b..88f54eb5a7c2 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -186,7 +186,7 @@ if ${no_local}; then
>         exit 0
>  fi
>
> -if ! test -e include/config/auto.conf; then
> +if ! test -e ${objtree}/include/config/auto.conf; then
>         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
>         exit 1
>  fi
> --
> 2.49.0.rc0.332.g42c0ae87b1-goog
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by Hong, Yifan 9 months, 1 week ago

On Wed, Mar 12, 2025 at 12:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Mar 12, 2025 at 11:12 AM HONG Yifan <elsk@google.com> wrote:
> >
> > setlocalversion reads include/config/auto.conf, which is located below
> > $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> > some in-kernel build artifacts").
> >
> > Hence, the setlocalversion script needs to use
> > $(objtree)/include/config/auto.conf as well.
> >
> > Note that $(objtree) is not necessarily `.` when O (aka KBUILD_OUTPUT)
> > is set, because of commit 13b25489b6f8 ("kbuild: change working
> > directory to external module directory with M=").
>
> Is this a real issue?
> If so, please attach some commands to reproduce an issue.
>
> setlocalversion is invoked only at line 1238 of the top-level Makefile,
> within the check "ifeq ($(KBUILD_EXTMOD),)"
> So, it is never called with external module builds.

Thanks Masahiro. You are right; this is not a real issue as the code
is right now.

In our case, we have this issue because we have
https://lore.kernel.org/all/20210121213641.3477522-1-willmcvicker@google.com/
("[PATCH v6] modules: introduce the MODULE_SCMVERSION config")
in our tree to support the SCM version for modules. The patch was not
accepted so
we applied this patch locally. Hence, technically this patch should also only be
applied locally by us. The paragraph that says "Note that $(objtree) is not
necessarily `.`..." is not correct.

Still, I think it makes sense to be consistent with other places that mentions
include/config/auto.conf.

Should I update the commit message and send another patch? Or would you
like to reject this change?

>
>
>
>
>
>
>
> > Signed-off-by: HONG Yifan <elsk@google.com>
> > ---
> > Implementation note: Should I test -z ${objtree} before using it? Otherwise it
> > looks at /include/config/auto.conf which is wrong.
> > Or should I fall back to `.` if objtree is not found in the environment
> > variables?
> > I could also `exit 1` if $objtree is not set. Please let me know what you think.
> >
> >  scripts/setlocalversion | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > index 28169d7e143b..88f54eb5a7c2 100755
> > --- a/scripts/setlocalversion
> > +++ b/scripts/setlocalversion
> > @@ -186,7 +186,7 @@ if ${no_local}; then
> >         exit 0
> >  fi
> >
> > -if ! test -e include/config/auto.conf; then
> > +if ! test -e ${objtree}/include/config/auto.conf; then
> >         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
> >         exit 1
> >  fi
> > --
> > 2.49.0.rc0.332.g42c0ae87b1-goog
> >
>
>
> --
> Best Regards
> Masahiro Yamada

Re: [PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by Masahiro Yamada 9 months, 1 week ago

On Thu, Mar 13, 2025 at 4:14 AM Hong, Yifan <elsk@google.com> wrote:
>
> On Wed, Mar 12, 2025 at 12:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Wed, Mar 12, 2025 at 11:12 AM HONG Yifan <elsk@google.com> wrote:
> > >
> > > setlocalversion reads include/config/auto.conf, which is located below
> > > $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> > > some in-kernel build artifacts").
> > >
> > > Hence, the setlocalversion script needs to use
> > > $(objtree)/include/config/auto.conf as well.
> > >
> > > Note that $(objtree) is not necessarily `.` when O (aka KBUILD_OUTPUT)
> > > is set, because of commit 13b25489b6f8 ("kbuild: change working
> > > directory to external module directory with M=").
> >
> > Is this a real issue?
> > If so, please attach some commands to reproduce an issue.
> >
> > setlocalversion is invoked only at line 1238 of the top-level Makefile,
> > within the check "ifeq ($(KBUILD_EXTMOD),)"
> > So, it is never called with external module builds.
>
> Thanks Masahiro. You are right; this is not a real issue as the code
> is right now.
>
> In our case, we have this issue because we have
> https://lore.kernel.org/all/20210121213641.3477522-1-willmcvicker@google.com/
> ("[PATCH v6] modules: introduce the MODULE_SCMVERSION config")
> in our tree to support the SCM version for modules. The patch was not
> accepted so
> we applied this patch locally. Hence, technically this patch should also only be
> applied locally by us. The paragraph that says "Note that $(objtree) is not
> necessarily `.`..." is not correct.
>
> Still, I think it makes sense to be consistent with other places that mentions
> include/config/auto.conf.
>
> Should I update the commit message and send another patch? Or would you
> like to reject this change?

OK, I will accept this if the commit message correctly describes this.

And, please fix up all the 3 occurrences for consistency.

Thanks.




>
> >
> >
> >
> >
> >
> >
> >
> > > Signed-off-by: HONG Yifan <elsk@google.com>
> > > ---
> > > Implementation note: Should I test -z ${objtree} before using it? Otherwise it
> > > looks at /include/config/auto.conf which is wrong.
> > > Or should I fall back to `.` if objtree is not found in the environment
> > > variables?
> > > I could also `exit 1` if $objtree is not set. Please let me know what you think.
> > >
> > >  scripts/setlocalversion | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > > index 28169d7e143b..88f54eb5a7c2 100755
> > > --- a/scripts/setlocalversion
> > > +++ b/scripts/setlocalversion
> > > @@ -186,7 +186,7 @@ if ${no_local}; then
> > >         exit 0
> > >  fi
> > >
> > > -if ! test -e include/config/auto.conf; then
> > > +if ! test -e ${objtree}/include/config/auto.conf; then
> > >         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
> > >         exit 1
> > >  fi
> > > --
> > > 2.49.0.rc0.332.g42c0ae87b1-goog
> > >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada



-- 
Best Regards
Masahiro Yamada

[PATCH v2] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by HONG Yifan 9 months ago

setlocalversion reads include/config/auto.conf, which is located below
$(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
some in-kernel build artifacts").

To be consistent, the setlocalversion script should use
${objtree}/include/config/auto.conf as well.

Signed-off-by: HONG Yifan <elsk@google.com>
---
v1: https://lore.kernel.org/lkml/20250312021154.102262-2-elsk@google.com/
v1 -> v2: fixed the other two locations of include/config/auto.conf in
setlocalversion script; also removed incorrect claim in commit message.

 scripts/setlocalversion | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 28169d7e143b..c13fe6e585e9 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -186,16 +186,16 @@ if ${no_local}; then
 	exit 0
 fi

-if ! test -e include/config/auto.conf; then
+if ! test -e ${objtree}/include/config/auto.conf; then
 	echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
 	exit 1
 fi

 # version string from CONFIG_LOCALVERSION
-config_localversion=$(sed -n 's/^CONFIG_LOCALVERSION=\(.*\)$/\1/p' include/config/auto.conf)
+config_localversion=$(sed -n 's/^CONFIG_LOCALVERSION=\(.*\)$/\1/p' ${objtree}/include/config/auto.conf)

 # scm version string if not at the kernel version tag or at the file_localversion
-if grep -q "^CONFIG_LOCALVERSION_AUTO=y$" include/config/auto.conf; then
+if grep -q "^CONFIG_LOCALVERSION_AUTO=y$" ${objtree}/include/config/auto.conf; then
 	# full scm version string
 	scm_version="$(scm_version)"
 elif [ "${LOCALVERSION+set}" != "set" ]; then
--
2.49.0.rc1.451.g8f38331e32-goog

Re: [PATCH v2] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by Masahiro Yamada 9 months ago

On Tue, Mar 18, 2025 at 9:59 AM HONG Yifan <elsk@google.com> wrote:
>
> setlocalversion reads include/config/auto.conf, which is located below
> $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> some in-kernel build artifacts").
>
> To be consistent, the setlocalversion script should use

"To be consistent" is too weak because we do not add
$(objtree)/ to include/config/auto.conf

Just run "git grep include/config/auto.conf"

You will see more include/config/auto.conf instances
that lack $(objtree)/ prefix.

So, "To be consistent" is not a reason.

You described why Google needs to have this
specifically scripts/setlocalversion.

Without that explained, I do not understand _why_.



> ${objtree}/include/config/auto.conf as well.
>
> Signed-off-by: HONG Yifan <elsk@google.com>
> ---
> v1: https://lore.kernel.org/lkml/20250312021154.102262-2-elsk@google.com/
> v1 -> v2: fixed the other two locations of include/config/auto.conf in
> setlocalversion script; also removed incorrect claim in commit message.
>
>  scripts/setlocalversion | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> index 28169d7e143b..c13fe6e585e9 100755
> --- a/scripts/setlocalversion
> +++ b/scripts/setlocalversion
> @@ -186,16 +186,16 @@ if ${no_local}; then
>         exit 0
>  fi
>
> -if ! test -e include/config/auto.conf; then
> +if ! test -e ${objtree}/include/config/auto.conf; then


Please quote

"${objtree}/include/config/auto.conf"

to avoid a shellcheck warning.




>         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
>         exit 1
>  fi
>
>  # version string from CONFIG_LOCALVERSION
> -config_localversion=$(sed -n 's/^CONFIG_LOCALVERSION=\(.*\)$/\1/p' include/config/auto.conf)
> +config_localversion=$(sed -n 's/^CONFIG_LOCALVERSION=\(.*\)$/\1/p' ${objtree}/include/config/auto.conf)
>
>  # scm version string if not at the kernel version tag or at the file_localversion
> -if grep -q "^CONFIG_LOCALVERSION_AUTO=y$" include/config/auto.conf; then
> +if grep -q "^CONFIG_LOCALVERSION_AUTO=y$" ${objtree}/include/config/auto.conf; then
>         # full scm version string
>         scm_version="$(scm_version)"
>  elif [ "${LOCALVERSION+set}" != "set" ]; then
> --
> 2.49.0.rc1.451.g8f38331e32-goog
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by Hong, Yifan 8 months, 4 weeks ago

On Sat, Mar 22, 2025 at 8:15 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Mar 18, 2025 at 9:59 AM HONG Yifan <elsk@google.com> wrote:
> >
> > setlocalversion reads include/config/auto.conf, which is located below
> > $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> > some in-kernel build artifacts").
> >
> > To be consistent, the setlocalversion script should use
>
> "To be consistent" is too weak because we do not add
> $(objtree)/ to include/config/auto.conf
>
> Just run "git grep include/config/auto.conf"
>
> You will see more include/config/auto.conf instances
> that lack $(objtree)/ prefix.
>
> So, "To be consistent" is not a reason.
>
> You described why Google needs to have this
> specifically scripts/setlocalversion.
>
> Without that explained, I do not understand _why_.
>

Without the context of Google's out-of-tree patch, I can't really
think of any good reason other than "To be consistent". Would it be
better if I quote
https://lore.kernel.org/all/20210121213641.3477522-1-willmcvicker@google.com/
("[PATCH v6] modules: introduce the MODULE_SCMVERSION config") in the
commit message, explaining that "prepending $(objtree)/ is required
for this other patch to work"?

>
>
> > ${objtree}/include/config/auto.conf as well.
> >
> > Signed-off-by: HONG Yifan <elsk@google.com>
> > ---
> > v1: https://lore.kernel.org/lkml/20250312021154.102262-2-elsk@google.com/
> > v1 -> v2: fixed the other two locations of include/config/auto.conf in
> > setlocalversion script; also removed incorrect claim in commit message.
> >
> >  scripts/setlocalversion | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > index 28169d7e143b..c13fe6e585e9 100755
> > --- a/scripts/setlocalversion
> > +++ b/scripts/setlocalversion
> > @@ -186,16 +186,16 @@ if ${no_local}; then
> >         exit 0
> >  fi
> >
> > -if ! test -e include/config/auto.conf; then
> > +if ! test -e ${objtree}/include/config/auto.conf; then
>
>
> Please quote
>
> "${objtree}/include/config/auto.conf"
>
> to avoid a shellcheck warning.
>
>
>
>
> >         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
> >         exit 1
> >  fi
> >
> >  # version string from CONFIG_LOCALVERSION
> > -config_localversion=$(sed -n 's/^CONFIG_LOCALVERSION=\(.*\)$/\1/p' include/config/auto.conf)
> > +config_localversion=$(sed -n 's/^CONFIG_LOCALVERSION=\(.*\)$/\1/p' ${objtree}/include/config/auto.conf)
> >
> >  # scm version string if not at the kernel version tag or at the file_localversion
> > -if grep -q "^CONFIG_LOCALVERSION_AUTO=y$" include/config/auto.conf; then
> > +if grep -q "^CONFIG_LOCALVERSION_AUTO=y$" ${objtree}/include/config/auto.conf; then
> >         # full scm version string
> >         scm_version="$(scm_version)"
> >  elif [ "${LOCALVERSION+set}" != "set" ]; then
> > --
> > 2.49.0.rc1.451.g8f38331e32-goog
> >
>
>
> --
> Best Regards
> Masahiro Yamada

Re: [PATCH v1] setlocalversion: use ${objtree}/include/config/auto.conf

Posted by Hong, Yifan 9 months, 1 week ago

On Wed, Mar 12, 2025 at 12:13 PM Hong, Yifan <elsk@google.com> wrote:
>
> On Wed, Mar 12, 2025 at 12:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Wed, Mar 12, 2025 at 11:12 AM HONG Yifan <elsk@google.com> wrote:
> > >
> > > setlocalversion reads include/config/auto.conf, which is located below
> > > $(objtree) with commit 214c0eea43b2 ("kbuild: add $(objtree)/ prefix to
> > > some in-kernel build artifacts").
> > >
> > > Hence, the setlocalversion script needs to use
> > > $(objtree)/include/config/auto.conf as well.
> > >
> > > Note that $(objtree) is not necessarily `.` when O (aka KBUILD_OUTPUT)
> > > is set, because of commit 13b25489b6f8 ("kbuild: change working
> > > directory to external module directory with M=").
> >
> > Is this a real issue?
> > If so, please attach some commands to reproduce an issue.
> >
> > setlocalversion is invoked only at line 1238 of the top-level Makefile,
> > within the check "ifeq ($(KBUILD_EXTMOD),)"
> > So, it is never called with external module builds.
>
> Thanks Masahiro. You are right; this is not a real issue as the code
> is right now.
>
> In our case, we have this issue because we have
> https://lore.kernel.org/all/20210121213641.3477522-1-willmcvicker@google.com/
> ("[PATCH v6] modules: introduce the MODULE_SCMVERSION config")
> in our tree to support the SCM version for modules. The patch was not
> accepted so
> we applied this patch locally. Hence, technically this patch should also only be
> applied locally by us. The paragraph that says "Note that $(objtree) is not
> necessarily `.`..." is not correct.
>
> Still, I think it makes sense to be consistent with other places that mentions
> include/config/auto.conf.
>
> Should I update the commit message and send another patch? Or would you
> like to reject this change?
>
> >
> >
> >
> >
> >
> >
> >
> > > Signed-off-by: HONG Yifan <elsk@google.com>
> > > ---
> > > Implementation note: Should I test -z ${objtree} before using it? Otherwise it
> > > looks at /include/config/auto.conf which is wrong.
> > > Or should I fall back to `.` if objtree is not found in the environment
> > > variables?
> > > I could also `exit 1` if $objtree is not set. Please let me know what you think.
> > >
> > >  scripts/setlocalversion | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > > index 28169d7e143b..88f54eb5a7c2 100755
> > > --- a/scripts/setlocalversion
> > > +++ b/scripts/setlocalversion
> > > @@ -186,7 +186,7 @@ if ${no_local}; then
> > >         exit 0
> > >  fi
> > >
> > > -if ! test -e include/config/auto.conf; then
> > > +if ! test -e ${objtree}/include/config/auto.conf; then
> > >         echo "Error: kernelrelease not valid - run 'make prepare' to update it" >&2
> > >         exit 1
> > >  fi

Actually, I noticed two more include/config/auto.conf mentions below
this line. If the overall idea looks good to you, I'll upload a
separate version that fixes the two mentions below as well.

> > > --
> > > 2.49.0.rc0.332.g42c0ae87b1-goog
> > >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada