[PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

This patch appends the SHA1 hash of the current git tree to the
kernel version line, or "[Not git tree]" if run from a non-git tree.
Uses "git log" to print the hash.

Suggested-by: Ingo Molnar <mingo@elte.hu>
Cc: Roman Zippel <zippel@linux-m68k.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 confdata.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c4dec80..4bd7842 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -399,10 +399,11 @@ int conf_read(const char *name)
 int conf_write(const char *name)
 {
 	FILE *out;
+	FILE *git;
 	struct symbol *sym;
 	struct menu *menu;
 	const char *basename;
-	char dirname[128], tmpname[128], newname[128];
+	char dirname[128], tmpname[128], newname[128], gitsha[128];
 	int type, l;
 	const char *str;
 	time_t now;
@@ -450,12 +451,20 @@ int conf_write(const char *name)
 	if (env && *env)
 		use_timestamp = 0;
 
+	gitsha[0] = '\0';
+	git = popen("git log --pretty=format:%h -1 2> /dev/null", "r");
+	if (git != NULL) {
+		fscanf(git, " %127s ", gitsha);
+		pclose(git);
+	}
+
 	fprintf(out, _("#\n"
 		       "# Automatically generated make config: don't edit\n"
-		       "# Linux kernel version: %s\n"
+		       "# Linux kernel version: %s %s\n"
 		       "%s%s"
 		       "#\n"),
 		     sym_get_string_value(sym),
+		     gitsha[0] == '\0' ? "[Not git tree]" : gitsha,
 		     use_timestamp ? "# " : "",
 		     use_timestamp ? ctime(&now) : "");
 

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Ingo Molnar]

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> This patch appends the SHA1 hash of the current git tree to the
> kernel version line, or "[Not git tree]" if run from a non-git tree.
> Uses "git log" to print the hash.
> 
> Suggested-by: Ingo Molnar <mingo@elte.hu>
> Cc: Roman Zippel <zippel@linux-m68k.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

>  	fprintf(out, _("#\n"
>  		       "# Automatically generated make config: don't edit\n"
> -		       "# Linux kernel version: %s\n"
> +		       "# Linux kernel version: %s %s\n"
>  		       "%s%s"
>  		       "#\n"),
>  		     sym_get_string_value(sym),
> +		     gitsha[0] == '\0' ? "[Not git tree]" : gitsha,

Very nice!

I have scripting that does something similar for (most of) my own configs, but 
it would be nice to see your more complete patch upstream.

Thanks,

	Ingo

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Frans Pop]

Paul E. McKenney wrote:
> This patch appends the SHA1 hash of the current git tree to the
> kernel version line, or "[Not git tree]" if run from a non-git tree.
> Uses "git log" to print the hash.

Nice idea.

However, I gave it a try and got:
# Linux kernel version: 2.6.34-rc0 [Not git tree]

Even though I *am* building from a git tree. It does not seem to work when 
building with KBUILD_OUTPUT set or when using 'O='.

IMO that makes the practical value of the patch very limited. I'm afraid 
because of this it will increase confusion rather than add information.

Cheers,
FJP

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Geert Uytterhoeven]

On Mon, Mar 1, 2010 at 10:42, Frans Pop <elendil@planet.nl> wrote:
> Paul E. McKenney wrote:
>> This patch appends the SHA1 hash of the current git tree to the
>> kernel version line, or "[Not git tree]" if run from a non-git tree.
>> Uses "git log" to print the hash.
>
> Nice idea.
>
> However, I gave it a try and got:
> # Linux kernel version: 2.6.34-rc0 [Not git tree]
>
> Even though I *am* building from a git tree. It does not seem to work when
> building with KBUILD_OUTPUT set or when using 'O='.
>
> IMO that makes the practical value of the patch very limited. I'm afraid
> because of this it will increase confusion rather than add information.

scripts/setlocalversion handles this case correctly, so you may want to
look there...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Linus Torvalds]

On Sun, 28 Feb 2010, Paul E. McKenney wrote:
>
> Uses "git log" to print the hash.

Please don't use "git log" for something like this.

Sure, it works, but it's kind of silly to say "I want a log, except I want 
only a single entry, and btw, I don't actually even want the log entry for 
that single entry at all, just the hash".

It boils down to "I want a log, except with none of the log part". It 
should make you go "Do I really want a log"?

If you really want the hash, maybe just using "git rev-parse HEAD" would 
do it.

However, in this case I think _any_ of those would be wrong. Wouldn't it 
make sense to use the same thing that we already compute for 'uname' 
(scripts/setlocalversion)? Especially as that one already knows how to 
handle other SCM's too (ie the whole hg/svn parts).

That script also ends up using a nicer format, ie it uses "git describe" 
to give a better idea of where it all is.

				Linus

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Mon, Mar 01, 2010 at 11:10:54AM +0100, Geert Uytterhoeven wrote:
> On Mon, Mar 1, 2010 at 10:42, Frans Pop <elendil@planet.nl> wrote:
> > Paul E. McKenney wrote:
> >> This patch appends the SHA1 hash of the current git tree to the
> >> kernel version line, or "[Not git tree]" if run from a non-git tree.
> >> Uses "git log" to print the hash.
> >
> > Nice idea.
> >
> > However, I gave it a try and got:
> > # Linux kernel version: 2.6.34-rc0 [Not git tree]
> >
> > Even though I *am* building from a git tree. It does not seem to work when
> > building with KBUILD_OUTPUT set or when using 'O='.
> >
> > IMO that makes the practical value of the patch very limited. I'm afraid
> > because of this it will increase confusion rather than add information.
> 
> scripts/setlocalversion handles this case correctly, so you may want to
> look there...

Very nice, thank you!!!

Here is the updated patch.  Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

[PATCH RFC] kconfig: place localversion string in .config output

This patch appends the localversion string to the Linux kernel version.
For example, in a git tree with uncommitted changes, the .config file
might start as follows:

# Automatically generated make config: don't edit
# Linux kernel version: 2.6.33-01836-g90a6501-dirty
# Mon Mar  1 08:21:49 2010

This patch uses the scripts/setlocalversion output, so similar output
is also generated for svn and mercurial.

Suggested-by: Ingo Molnar <mingo@elte.hu>
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Frans Pop <elendil@planet.nl>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 confdata.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c4dec80..fba6b07 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -399,10 +399,11 @@ int conf_read(const char *name)
 int conf_write(const char *name)
 {
 	FILE *out;
+	FILE *slv;
 	struct symbol *sym;
 	struct menu *menu;
 	const char *basename;
-	char dirname[128], tmpname[128], newname[128];
+	char dirname[128], tmpname[128], newname[128], localversion[128];
 	int type, l;
 	const char *str;
 	time_t now;
@@ -450,12 +451,20 @@ int conf_write(const char *name)
 	if (env && *env)
 		use_timestamp = 0;
 
+	localversion[0] = '\0';
+	slv = popen("scripts/setlocalversion 2> /dev/null", "r");
+	if (slv != NULL) {
+		fscanf(slv, " %127s ", localversion);
+		pclose(slv);
+	}
+
 	fprintf(out, _("#\n"
 		       "# Automatically generated make config: don't edit\n"
-		       "# Linux kernel version: %s\n"
+		       "# Linux kernel version: %s%s\n"
 		       "%s%s"
 		       "#\n"),
 		     sym_get_string_value(sym),
+		     localversion[0] != '\0' ? localversion : "",
 		     use_timestamp ? "# " : "",
 		     use_timestamp ? ctime(&now) : "");
 

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Mon, Mar 01, 2010 at 08:22:15AM -0800, Linus Torvalds wrote:
> 
> 
> On Sun, 28 Feb 2010, Paul E. McKenney wrote:
> >
> > Uses "git log" to print the hash.
> 
> Please don't use "git log" for something like this.
> 
> Sure, it works, but it's kind of silly to say "I want a log, except I want 
> only a single entry, and btw, I don't actually even want the log entry for 
> that single entry at all, just the hash".
> 
> It boils down to "I want a log, except with none of the log part". It 
> should make you go "Do I really want a log"?
> 
> If you really want the hash, maybe just using "git rev-parse HEAD" would 
> do it.
> 
> However, in this case I think _any_ of those would be wrong. Wouldn't it 
> make sense to use the same thing that we already compute for 'uname' 
> (scripts/setlocalversion)? Especially as that one already knows how to 
> handle other SCM's too (ie the whole hg/svn parts).
> 
> That script also ends up using a nicer format, ie it uses "git describe" 
> to give a better idea of where it all is.

Agreed!  The "-dirty" modifier for the case of changes not yet
checked into git looks especially helpful.  Geert Uytterhoeven already
straightened me out on this one, and I posted an updated patch that
uses scripts/setlocalversion (as a reply to his email).

Still learning about git, and I suspect that I always will be in that
state.  ;-)

							Thanx, Paul

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Frans Pop]

On Monday 01 March 2010, Paul E. McKenney wrote:
> Here is the updated patch.  Thoughts?

AFAICT that still won't work with KBUILD_OUTPUT as you're not passing the 
source tree directory to the script. So it will default to "." and when 
KBUILD_OUTPUT is used that is not the source tree but the target tree (and 
thus not under git).

Cheers,
FJP

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Mon, Mar 01, 2010 at 05:53:32PM +0100, Frans Pop wrote:
> On Monday 01 March 2010, Paul E. McKenney wrote:
> > Here is the updated patch.  Thoughts?
> 
> AFAICT that still won't work with KBUILD_OUTPUT as you're not passing the 
> source tree directory to the script. So it will default to "." and when 
> KBUILD_OUTPUT is used that is not the source tree but the target tree (and 
> thus not under git).

Hmmm...  In that case, it won't find the scripts/setlocalversion script,
either.  Unless you have the git tree on your $PATH, which seems
unlikely.  So I can just check for popen() failure and take corrective
action.

Taking a look at conf_get_default_confname(), it looks like one approach
would be to prepend getenv(SRCTREE) to the name of the script and also
pass this to the script.  Will that work for your setup?  If so, please
see below for the updated patch.

							Thanx, Paul

------------------------------------------------------------------------

[PATCH RFC] kconfig: place localversion string in .config output

This patch appends the localversion string to the Linux kernel version.
For example, in a git tree with uncommitted changes, the .config file
might start as follows:

# Automatically generated make config: don't edit
# Linux kernel version: 2.6.33-01836-g90a6501-dirty
# Mon Mar  1 08:21:49 2010

This patch uses the scripts/setlocalversion output, so similar output
is also generated for svn and mercurial.

Suggested-by: Ingo Molnar <mingo@elte.hu>
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Frans Pop <elendil@planet.nl>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 confdata.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c4dec80..4c28ea5 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -399,10 +399,12 @@ int conf_read(const char *name)
 int conf_write(const char *name)
 {
 	FILE *out;
+	FILE *slv;
 	struct symbol *sym;
 	struct menu *menu;
 	const char *basename;
-	char dirname[128], tmpname[128], newname[128];
+	char dirname[128], tmpname[128], newname[128], localversion[128];
+	char cmdline[PATH_MAX * 2 + 128];
 	int type, l;
 	const char *str;
 	time_t now;
@@ -450,12 +452,27 @@ int conf_write(const char *name)
 	if (env && *env)
 		use_timestamp = 0;
 
+	localversion[0] = '\0';
+	slv = popen("scripts/setlocalversion 2> /dev/null", "r");
+	if (slv == NULL) {
+		env = getenv(SRCTREE);
+		if (env) {
+			sprintf(cmdline, "%s/scripts/setlocalversion %s 2> /dev/null", env, env);
+			slv = popen(cmdline, "r");
+		}
+	}
+	if (slv != NULL) {
+		fscanf(slv, " %127s ", localversion);
+		pclose(slv);
+	}
+
 	fprintf(out, _("#\n"
 		       "# Automatically generated make config: don't edit\n"
-		       "# Linux kernel version: %s\n"
+		       "# Linux kernel version: %s%s\n"
 		       "%s%s"
 		       "#\n"),
 		     sym_get_string_value(sym),
+		     localversion[0] != '\0' ? localversion : "",
 		     use_timestamp ? "# " : "",
 		     use_timestamp ? ctime(&now) : "");
 

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Frans Pop]

On Monday 01 March 2010, Paul E. McKenney wrote:
> Hmmm...  In that case, it won't find the scripts/setlocalversion script,
> either.  Unless you have the git tree on your $PATH, which seems
> unlikely.  So I can just check for popen() failure and take corrective
> action.

It doesn't work for me yet, but I don't see why not. It does work when I do 
e.g. a 'make defconfig' in the current directory, but not when I set 
KBUILD_OUTPUT.

It's probably easiest if you try to debug it yourself. It's as simple as:
$ mkdir /tmp/kbuild
$ KBUILD_OUTPUT=/tmp/kbuild make defconfig


One other thing. I wonder if this implementation will always reliably 
result in the *current* SHA1 being included in the .config. AFAICT 
the .config only actually gets written if there are changes, or if you 
explicitly do a 'make oldconfig'.

But if you e.g. pull a stable update and just run 'make', the .config will 
likely remain unchanged and will thus still contain the SHA1 from a 
previous build.

Cheers,
FJP

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[James Cloos]

>>>>> "Paul" == Paul E McKenney <paulmck@linux.vnet.ibm.com> writes:

Paul> Agreed!  The "-dirty" modifier for the case of changes not yet
Paul> checked into git looks especially helpful.

Except that the script calls »git update-index --refresh --unmerged«
and »git diff-index --name-only HEAD«, both of which are painfully
slow and resource intensive.

I'd hate to have that run every time I make a kernel.

-JimC
-- 
James Cloos <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Mon, Mar 01, 2010 at 09:29:35PM +0100, Frans Pop wrote:
> On Monday 01 March 2010, Paul E. McKenney wrote:
> > Hmmm...  In that case, it won't find the scripts/setlocalversion script,
> > either.  Unless you have the git tree on your $PATH, which seems
> > unlikely.  So I can just check for popen() failure and take corrective
> > action.
> 
> It doesn't work for me yet, but I don't see why not. It does work when I do 
> e.g. a 'make defconfig' in the current directory, but not when I set 
> KBUILD_OUTPUT.

Sigh!  Because popen() doesn't fail when the command is bogus, it seems.
I now do stat() to check for it existing, and that seems to handle the
remote-output case.

> It's probably easiest if you try to debug it yourself. It's as simple as:
> $ mkdir /tmp/kbuild
> $ KBUILD_OUTPUT=/tmp/kbuild make defconfig

Thank you!!!

> One other thing. I wonder if this implementation will always reliably 
> result in the *current* SHA1 being included in the .config. AFAICT 
> the .config only actually gets written if there are changes, or if you 
> explicitly do a 'make oldconfig'.
> 
> But if you e.g. pull a stable update and just run 'make', the .config will 
> likely remain unchanged and will thus still contain the SHA1 from a 
> previous build.

Good point.  But the same is true of the Linux kernel version
identifier, right?

Anyway, here is the updated patch.

							Thanx, Paul

------------------------------------------------------------------------

kconfig: place localversion string in .config output

This patch appends the localversion string to the Linux kernel version.
For example, in a git tree with uncommitted changes, the .config file
might start as follows (but with leading hash marks):

	Automatically generated make config: don't edit
	Linux kernel version: 2.6.33-01836-g90a6501-dirty
	Mon Mar  1 17:05:59 2010

This patch uses the scripts/setlocalversion output, so similar output
is also generated for svn and mercurial.

Suggested-by: Ingo Molnar <mingo@elte.hu>
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Frans Pop <elendil@planet.nl>
Signed-off-by: Paul E.  McKenney <paulmck@linux.vnet.ibm.com>
---

 confdata.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c4dec80..6c067ab 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -399,15 +399,18 @@ int conf_read(const char *name)
 int conf_write(const char *name)
 {
 	FILE *out;
+	FILE *slv;
 	struct symbol *sym;
 	struct menu *menu;
 	const char *basename;
-	char dirname[128], tmpname[128], newname[128];
+	char dirname[128], tmpname[128], newname[128], localversion[128];
+	char cmdline[PATH_MAX * 2 + 128];
 	int type, l;
 	const char *str;
 	time_t now;
 	int use_timestamp = 1;
 	char *env;
+	struct stat statbuf;
 
 	dirname[0] = 0;
 	if (name && name[0]) {
@@ -450,12 +453,27 @@ int conf_write(const char *name)
 	if (env && *env)
 		use_timestamp = 0;
 
+	localversion[0] = '\0';
+	strcpy(cmdline, "scripts/setlocalversion 2> /dev/null");
+	if (stat("scripts/setlocalversion", &statbuf) != 0) {
+		env = getenv("KBUILD_SRC");
+		if (env) {
+			sprintf(cmdline, "%s/scripts/setlocalversion %s 2> /dev/null", env, env);
+		}
+	}
+	slv = popen(cmdline, "r");
+	if (slv != NULL) {
+		fscanf(slv, " %127s ", localversion);
+		pclose(slv);
+	}
+
 	fprintf(out, _("#\n"
 		       "# Automatically generated make config: don't edit\n"
-		       "# Linux kernel version: %s\n"
+		       "# Linux kernel version: %s%s\n"
 		       "%s%s"
 		       "#\n"),
 		     sym_get_string_value(sym),
+		     localversion[0] != '\0' ? localversion : "",
 		     use_timestamp ? "# " : "",
 		     use_timestamp ? ctime(&now) : "");
 

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Mon, Mar 01, 2010 at 03:46:26PM -0500, James Cloos wrote:
> >>>>> "Paul" == Paul E McKenney <paulmck@linux.vnet.ibm.com> writes:
> 
> Paul> Agreed!  The "-dirty" modifier for the case of changes not yet
> Paul> checked into git looks especially helpful.
> 
> Except that the script calls »git update-index --refresh --unmerged«
> and »git diff-index --name-only HEAD«, both of which are painfully
> slow and resource intensive.
> 
> I'd hate to have that run every time I make a kernel.

Good point...  Should we have an environment variable that controls this
behavior?

							Thanx, Paul

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[James Cloos]

>>>>> "Paul" == Paul E McKenney <paulmck@linux.vnet.ibm.com> writes:

Paul> Agreed!  The "-dirty" modifier for the case of changes not yet
Paul> checked into git looks especially helpful.

JimC> Except that the script calls »git update-index --refresh --unmerged«
JimC> and »git diff-index --name-only HEAD«, both of which are painfully
JimC> slow and resource intensive.

JimC> I'd hate to have that run every time I make a kernel.

Paul> Good point...  Should we have an environment variable that controls
Paul> this behavior?

I wouldn't mind the intial idea: just the abbreviated top of three hash.

I was, next, going to write that CONFIG_LOCALVERSION_AUTO already exists
for that, but LOCALVERSION_AUTO calls scripts/setlocalversion these days
and that is what I'd prefer to avoid.  [SIGH]

Shows how long it has been since I last used it.  I stopped using it
when I was maintaining my .config in git.  Now I maintain .config in a
separate git repo, for the express purpose of avoiding a merge (rather
than a fast forward) when I run git pull.  Something that a local edit
of scripts/setlocalversion will, of course, prevent.

Anyone up for a CONFIG_LOCALVERSION_AUTO_DO_NOT_BOTHER_WITH_DIRTY? ☺

-JimC
-- 
James Cloos <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Mon, Mar 01, 2010 at 08:53:19PM -0500, James Cloos wrote:
> >>>>> "Paul" == Paul E McKenney <paulmck@linux.vnet.ibm.com> writes:
> 
> Paul> Agreed!  The "-dirty" modifier for the case of changes not yet
> Paul> checked into git looks especially helpful.
> 
> JimC> Except that the script calls »git update-index --refresh --unmerged«
> JimC> and »git diff-index --name-only HEAD«, both of which are painfully
> JimC> slow and resource intensive.
> 
> JimC> I'd hate to have that run every time I make a kernel.
> 
> Paul> Good point...  Should we have an environment variable that controls
> Paul> this behavior?
> 
> I wouldn't mind the intial idea: just the abbreviated top of three hash.
> 
> I was, next, going to write that CONFIG_LOCALVERSION_AUTO already exists
> for that, but LOCALVERSION_AUTO calls scripts/setlocalversion these days
> and that is what I'd prefer to avoid.  [SIGH]
> 
> Shows how long it has been since I last used it.  I stopped using it
> when I was maintaining my .config in git.  Now I maintain .config in a
> separate git repo, for the express purpose of avoiding a merge (rather
> than a fast forward) when I run git pull.  Something that a local edit
> of scripts/setlocalversion will, of course, prevent.
> 
> Anyone up for a CONFIG_LOCALVERSION_AUTO_DO_NOT_BOTHER_WITH_DIRTY? ☺

It looks like it would be pretty easy to make make scripts/setlocalversion
look at an environment variable to decide whether or not to do the index
update and the check for uncommitted changes.  Default would seem to
need to be to do it the slow way.

Or would that cause other problems?

							Thanx, Paul

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Frans Pop]

On Tuesday 02 March 2010, Paul E. McKenney wrote:
> Sigh!  Because popen() doesn't fail when the command is bogus, it seems.
> I now do stat() to check for it existing, and that seems to handle the
> remote-output case.

Cool. Did you check if it's really necessary to treat the two cases 
separately? I would expect srctree to always be set, and if I read the 
root Makefile correctly that's indeed the case:
srctree         := $(if $(KBUILD_SRC),$(KBUILD_SRC),$(CURDIR))

Hmm. I see you've switched from SRCTREE to KBUILD_SRC. Is that correct? 
SCRTREE is also used elsewhere in the same file and that's the variable 
(in lower case; the upper case variant is a C define) that's explicitly 
exported in the root Makefile.

> > One other thing. I wonder if this implementation will always reliably
> > result in the *current* SHA1 being included in the .config. AFAICT
> > the .config only actually gets written if there are changes, or if you
> > explicitly do a 'make oldconfig'.
> >
> > But if you e.g. pull a stable update and just run 'make', the .config
> > will likely remain unchanged and will thus still contain the SHA1 from
> > a previous build.
>
> Good point.  But the same is true of the Linux kernel version
> identifier, right?

Right. But the increased precision of the SHA1 IMO also demands an 
increased reliability of its accuracy: if it's not accurate it loses its 
value.

Personally I think that maybe the config file is not the correct place for 
this. For one thing users may well send a config file that's not the 100% 
exact one used to build the kernel when they know it's close enough that 
it makes no practical difference (I know I've done so in the past).

Wouldn't it be more logical to include the line in the dmesg output? My 
preference would be a separate line below the existing (Linux version) 
line. That line could only be output if the kernel was built from a VCS.
It could then even be repeated in oops output.

That would also solve the problem of the config file not being updated.

Paul E. McKenney wrote:
>> I'd hate to have that run every time I make a kernel.
> 
> Good point...  Should we have an environment variable that controls this
> behavior?

I would suggest at most a general enabled/disabled switch (distros might 
prefer to disable it too), and not more detailed settings to control 
whether or not to include dirty or whatnot.

Cheers,
FJP

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Tue, Mar 02, 2010 at 04:19:32PM +0100, Frans Pop wrote:
> On Tuesday 02 March 2010, Paul E. McKenney wrote:
> > Sigh!  Because popen() doesn't fail when the command is bogus, it seems.
> > I now do stat() to check for it existing, and that seems to handle the
> > remote-output case.
> 
> Cool. Did you check if it's really necessary to treat the two cases 
> separately? I would expect srctree to always be set, and if I read the 
> root Makefile correctly that's indeed the case:
> srctree         := $(if $(KBUILD_SRC),$(KBUILD_SRC),$(CURDIR))

You are quite correct, the environment variable is set correctly either
way.  I have therefore switched to the single case as you suggest.

> Hmm. I see you've switched from SRCTREE to KBUILD_SRC. Is that correct? 
> SCRTREE is also used elsewhere in the same file and that's the variable 
> (in lower case; the upper case variant is a C define) that's explicitly 
> exported in the root Makefile.

They both work, but agreed, SRCTREE is what is used elsewhere in the
file, so I have switched back to SRCTREE.  The move to "KBUILD_SRC"
was part of my debugging work.

> > > One other thing. I wonder if this implementation will always reliably
> > > result in the *current* SHA1 being included in the .config. AFAICT
> > > the .config only actually gets written if there are changes, or if you
> > > explicitly do a 'make oldconfig'.
> > >
> > > But if you e.g. pull a stable update and just run 'make', the .config
> > > will likely remain unchanged and will thus still contain the SHA1 from
> > > a previous build.
> >
> > Good point.  But the same is true of the Linux kernel version
> > identifier, right?
> 
> Right. But the increased precision of the SHA1 IMO also demands an 
> increased reliability of its accuracy: if it's not accurate it loses its 
> value.

It does depend on the context -- many test setups do a config for each
build, and are thus guaranteed accurate.

This is another good argument for making this behavior non-default, the
other argument mentioned earlier in this thread being performance.

> Personally I think that maybe the config file is not the correct place for 
> this. For one thing users may well send a config file that's not the 100% 
> exact one used to build the kernel when they know it's close enough that 
> it makes no practical difference (I know I've done so in the past).
> 
> Wouldn't it be more logical to include the line in the dmesg output? My 
> preference would be a separate line below the existing (Linux version) 
> line. That line could only be output if the kernel was built from a VCS.
> It could then even be repeated in oops output.

My concern with only putting it in the dmesg output is that people do
not always capture that.  The oops output is more often captured, but
the kernel only has this information if CONFIG_LOCALVERSION_AUTO is set.

> That would also solve the problem of the config file not being updated.

Another approach would be to append the $(kernelrelease) make variable
to the end of the .config file when building the kernel.release file.
Unfortunately, as noted earlier in this thread, this only happens when
CONFIG_LOCALVERSION_AUTO is set.  And if people are setting such
variables, then they most likely have set things up to keep the version
accurate, in which case putting it at the top of the .config file makes
more sense.

I will send the updated patch in a separate email.

Also dumping the version (whatever it is) during early boot and as part of
oops messages sounds reasonable at first glance.  This could be done as
an independent change, if desired.  Other thoughts on this?

							Thanx, Paul

> Paul E. McKenney wrote:
> >> I'd hate to have that run every time I make a kernel.
> > 
> > Good point...  Should we have an environment variable that controls this
> > behavior?
> 
> I would suggest at most a general enabled/disabled switch (distros might 
> prefer to disable it too), and not more detailed settings to control 
> whether or not to include dirty or whatnot.
> 
> Cheers,
> FJP

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Frans Pop]

On Wednesday 03 March 2010, Paul E. McKenney wrote:
> > Wouldn't it be more logical to include the line in the dmesg output?
> > My preference would be a separate line below the existing (Linux
> > version) line. That line could only be output if the kernel was built
> > from a VCS. It could then even be repeated in oops output.
>
> My concern with only putting it in the dmesg output is that people do
> not always capture that.  The oops output is more often captured, but
> the kernel only has this information if CONFIG_LOCALVERSION_AUTO is set.

Yes, my suggestion is exactly because IMO it should be independent of 
CONFIG_LOCALVERSION_AUTO.

I hugely dislike that option because it makes the git version part of the 
kernel version and thus affects how the kernel gets installed (names of 
files in /boot, name of the directory in /lib/modules, name of the Debian 
package created using the deb-pkg target, etc.).
For all those things I want a "clean" kernel version and thus I will never 
enable CONFIG_LOCALVERSION_AUTO.

But I do see the value of a reliable and consistent identification of what 
exact source a kernel was built from. Including the git version separately 
from the kernel version would allow that.

Cheers,
FJP

Re: [PATCH RFC] kconfig: place git SHA1 in .config output if in git tree

[Paul E. McKenney]

On Wed, Mar 03, 2010 at 01:42:41AM +0100, Frans Pop wrote:
> On Wednesday 03 March 2010, Paul E. McKenney wrote:
> > > Wouldn't it be more logical to include the line in the dmesg output?
> > > My preference would be a separate line below the existing (Linux
> > > version) line. That line could only be output if the kernel was built
> > > from a VCS. It could then even be repeated in oops output.
> >
> > My concern with only putting it in the dmesg output is that people do
> > not always capture that.  The oops output is more often captured, but
> > the kernel only has this information if CONFIG_LOCALVERSION_AUTO is set.
> 
> Yes, my suggestion is exactly because IMO it should be independent of 
> CONFIG_LOCALVERSION_AUTO.
> 
> I hugely dislike that option because it makes the git version part of the 
> kernel version and thus affects how the kernel gets installed (names of 
> files in /boot, name of the directory in /lib/modules, name of the Debian 
> package created using the deb-pkg target, etc.).
> For all those things I want a "clean" kernel version and thus I will never 
> enable CONFIG_LOCALVERSION_AUTO.

That does sound a bit painful...

> But I do see the value of a reliable and consistent identification of what 
> exact source a kernel was built from. Including the git version separately 
> from the kernel version would allow that.

Fortunately, scripts/setlocalversion seems to run quite a bit faster the
second time I run it, probably due to various caches having been warmed
up the first time.  Because doing all of this straightforwardly means up
to three invocations in a given kernel build.  ;-)

							Thanx, Paul