diff mbox series

[2/2] kconfig: do not write .config if the content is the same

Message ID 20190510061205.28753-2-yamada.masahiro@socionext.com
State New
Headers show
Series [1/2] kconfig: do not accept a directory for configuration output | expand

Commit Message

Masahiro Yamada May 10, 2019, 6:12 a.m. UTC 
Kconfig updates the .config when it exits even if its content is
exactly the same as before. Since its timestamp becomes newer than
that of other build artifacts, additional processing is invoked,
which is annoying.

- syncconfig is invoked to update include/config/auto.conf, etc.

- kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,
  then vmlinux is relinked as well.

If the .config is not changed at all, we do not have to even
touch it. Just bail out showing "No change to .config".

  $ make allmodconfig
  scripts/kconfig/conf  --allmodconfig Kconfig
  #
  # configuration written to .config
  #
  $ make allmodconfig
  scripts/kconfig/conf  --allmodconfig Kconfig
  #
  # No change to .config
  #

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 scripts/kconfig/confdata.c | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

-- 
2.17.1

Comments

Greg Kroah-Hartman May 10, 2019, 6:21 a.m. UTC | #1 
On Fri, May 10, 2019 at 03:12:05PM +0900, Masahiro Yamada wrote:
> Kconfig updates the .config when it exits even if its content is

> exactly the same as before. Since its timestamp becomes newer than

> that of other build artifacts, additional processing is invoked,

> which is annoying.

> 

> - syncconfig is invoked to update include/config/auto.conf, etc.

> 

> - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,

>   then vmlinux is relinked as well.

> 

> If the .config is not changed at all, we do not have to even

> touch it. Just bail out showing "No change to .config".

> 

>   $ make allmodconfig

>   scripts/kconfig/conf  --allmodconfig Kconfig

>   #

>   # configuration written to .config

>   #

>   $ make allmodconfig

>   scripts/kconfig/conf  --allmodconfig Kconfig

>   #

>   # No change to .config

>   #

> 

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Reported-by: Linus Torvalds <torvalds@linux-foundation.org> ?

:)

Anyway, nice change, looks good to me:


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Geert Uytterhoeven May 10, 2019, 6:46 a.m. UTC | #2 
Hi Yamada-san,

On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Kconfig updates the .config when it exits even if its content is

> exactly the same as before. Since its timestamp becomes newer than

> that of other build artifacts, additional processing is invoked,

> which is annoying.

>

> - syncconfig is invoked to update include/config/auto.conf, etc.

>

> - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,

>   then vmlinux is relinked as well.

>

> If the .config is not changed at all, we do not have to even

> touch it. Just bail out showing "No change to .config".

>

>   $ make allmodconfig

>   scripts/kconfig/conf  --allmodconfig Kconfig

>   #

>   # configuration written to .config

>   #

>   $ make allmodconfig

>   scripts/kconfig/conf  --allmodconfig Kconfig

>   #

>   # No change to .config

>   #

>

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Thanks for your patch!

> --- a/scripts/kconfig/confdata.c

> +++ b/scripts/kconfig/confdata.c


> @@ -888,6 +935,13 @@ int conf_write(const char *name)

>         fclose(out);

>

>         if (*tmpname) {

> +               if (is_same(name, tmpname)) {

> +                       conf_message("No change to %s", name);

> +                       unlink(tmpname);

> +                       sym_set_change_count(0);

> +                       return 0;

> +               }

> +

>                 snprintf(oldname, sizeof(oldname), "%s.old", name);

>                 rename(name, oldname);

>                 if (rename(tmpname, name))


This causes a semantic change for the meaning of ".config.old", which is
no longer updated if .config has not changed.
Hence its contents may no longer correspond to the previous config, but to
an arbitrary older version.

My workflow involves always running my own script "linux-oldconfig",
instead of "make oldconfig", so I immediately see what has changed:

    $ cat $(type -p linux-oldconfig)
    #!/bin/bash
    make ${0#*/linux-} && colordiff -u .config{.old,}

However, after your patch, "linux-oldconfig" keeps on showing the old
changes, while .config itself has not changed.  I guess other people
tracking .config changes are using a similar scheme.

One way to fix this is to rename tmpname to oldname if is_same() returns
true. However, this may have side effects for other people, as the
timestamp of .config.old will be changed.

Do you have a better solution?
Thanks!

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
Sam Ravnborg May 10, 2019, 7:03 a.m. UTC | #3 
Hi Geert/Masahiro.

On Fri, May 10, 2019 at 08:46:35AM +0200, Geert Uytterhoeven wrote:
> Hi Yamada-san,

> 

> On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

> > Kconfig updates the .config when it exits even if its content is

> > exactly the same as before. Since its timestamp becomes newer than

> > that of other build artifacts, additional processing is invoked,

> > which is annoying.

> >

> > - syncconfig is invoked to update include/config/auto.conf, etc.

> >

> > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,

> >   then vmlinux is relinked as well.

> >

> > If the .config is not changed at all, we do not have to even

> > touch it. Just bail out showing "No change to .config".

It would be preferable that if nothing changed no output is generated.
Like we do not tell that we did not build a .o file because the .c file
had not changed.
Less noise for a kernel build where nothings happens.

> This causes a semantic change for the meaning of ".config.old", which is

> no longer updated if .config has not changed.

> Hence its contents may no longer correspond to the previous config, but to

> an arbitrary older version.

This semantic change is good.
So we now have a .config.old that correspond to the state before
the last change. Also after several kernel builds.

> > My workflow involves always running my own script "linux-oldconfig",

> instead of "make oldconfig", so I immediately see what has changed:

> 

>     $ cat $(type -p linux-oldconfig)

>     #!/bin/bash

>     make ${0#*/linux-} && colordiff -u .config{.old,}

So scripts relying on the old (broken) behaviour will no longer work.
The new behaviour is better as it is usefaul in many typical situations.

Hacking, hack. What did I change in the config?

	Sam
Masahiro Yamada May 10, 2019, 7:14 a.m. UTC | #4 
On Fri, May 10, 2019 at 3:21 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> On Fri, May 10, 2019 at 03:12:05PM +0900, Masahiro Yamada wrote:

> > Kconfig updates the .config when it exits even if its content is

> > exactly the same as before. Since its timestamp becomes newer than

> > that of other build artifacts, additional processing is invoked,

> > which is annoying.

> >

> > - syncconfig is invoked to update include/config/auto.conf, etc.

> >

> > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,

> >   then vmlinux is relinked as well.

> >

> > If the .config is not changed at all, we do not have to even

> > touch it. Just bail out showing "No change to .config".

> >

> >   $ make allmodconfig

> >   scripts/kconfig/conf  --allmodconfig Kconfig

> >   #

> >   # configuration written to .config

> >   #

> >   $ make allmodconfig

> >   scripts/kconfig/conf  --allmodconfig Kconfig

> >   #

> >   # No change to .config

> >   #

> >

> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>

> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> ?


OK.

For reviewers who do not know the context,
please see this comment from Linus:

https://lkml.org/lkml/2019/5/9/983



-- 
Best Regards
Masahiro Yamada
Geert Uytterhoeven May 10, 2019, 7:24 a.m. UTC | #5 
Hi Sam,

On Fri, May 10, 2019 at 9:03 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> On Fri, May 10, 2019 at 08:46:35AM +0200, Geert Uytterhoeven wrote:

> > On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada

> > <yamada.masahiro@socionext.com> wrote:

> > > Kconfig updates the .config when it exits even if its content is

> > > exactly the same as before. Since its timestamp becomes newer than

> > > that of other build artifacts, additional processing is invoked,

> > > which is annoying.

> > >

> > > - syncconfig is invoked to update include/config/auto.conf, etc.

> > >

> > > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,

> > >   then vmlinux is relinked as well.

> > >

> > > If the .config is not changed at all, we do not have to even

> > > touch it. Just bail out showing "No change to .config".


> > This causes a semantic change for the meaning of ".config.old", which is

> > no longer updated if .config has not changed.

> > Hence its contents may no longer correspond to the previous config, but to

> > an arbitrary older version.

> This semantic change is good.

> So we now have a .config.old that correspond to the state before

> the last change. Also after several kernel builds.

>

> > > My workflow involves always running my own script "linux-oldconfig",

> > instead of "make oldconfig", so I immediately see what has changed:

> >

> >     $ cat $(type -p linux-oldconfig)

> >     #!/bin/bash

> >     make ${0#*/linux-} && colordiff -u .config{.old,}

> So scripts relying on the old (broken) behaviour will no longer work.

> The new behaviour is better as it is usefaul in many typical situations.

>

> Hacking, hack. What did I change in the config?


I agree both semantics have their merits.
Sometimes I also wanted to see the last real change...

No worries, updating my script, so it works with both semantics:

    $ cat $(type -p linux-oldconfig)
    #!/bin/bash
    cp -a .config .config.orig
    make ${0#*/linux-} && colordiff -u .config{.orig,}

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
Masahiro Yamada May 10, 2019, 7:41 a.m. UTC | #6 
Hi Sam, Geert,

On Fri, May 10, 2019 at 4:04 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>

> Hi Geert/Masahiro.

>

> On Fri, May 10, 2019 at 08:46:35AM +0200, Geert Uytterhoeven wrote:

> > Hi Yamada-san,

> >

> > On Fri, May 10, 2019 at 8:14 AM Masahiro Yamada

> > <yamada.masahiro@socionext.com> wrote:

> > > Kconfig updates the .config when it exits even if its content is

> > > exactly the same as before. Since its timestamp becomes newer than

> > > that of other build artifacts, additional processing is invoked,

> > > which is annoying.

> > >

> > > - syncconfig is invoked to update include/config/auto.conf, etc.

> > >

> > > - kernel/config.o is recompiled if CONFIG_IKCONFIG is enabled,

> > >   then vmlinux is relinked as well.

> > >

> > > If the .config is not changed at all, we do not have to even

> > > touch it. Just bail out showing "No change to .config".

> It would be preferable that if nothing changed no output is generated.

> Like we do not tell that we did not build a .o file because the .c file

> had not changed.

> Less noise for a kernel build where nothings happens.

>

> > This causes a semantic change for the meaning of ".config.old", which is

> > no longer updated if .config has not changed.

> > Hence its contents may no longer correspond to the previous config, but to

> > an arbitrary older version.

> This semantic change is good.

> So we now have a .config.old that correspond to the state before

> the last change. Also after several kernel builds.



I agree.

When there is no change in the configuration,
Kconfig will not even attempt to output anything.

Updating only .config.old is strange.

Thanks.



--
Best Regards
Masahiro Yamada
diff mbox series

Patch

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 9fd6430c93d2..399973e35533 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -3,6 +3,7 @@ 
  * Copyright (C) 2002 Roman Zippel <zippel@linux-m68k.org>
  */
 
+#include <sys/mman.h>
 #include <sys/stat.h>
 #include <ctype.h>
 #include <errno.h>
@@ -36,6 +37,52 @@  static bool is_dir(const char *path)
 	return S_ISDIR(st.st_mode);
 }
 
+/* return true if the given two files are the same, false otherwise */
+static bool is_same(const char *file1, const char *file2)
+{
+	int fd1, fd2;
+	struct stat st1, st2;
+	void *map1, *map2;
+	bool ret = false;
+
+	fd1 = open(file1, O_RDONLY);
+	if (fd1 < 0)
+		return ret;
+
+	fd2 = open(file2, O_RDONLY);
+	if (fd2 < 0)
+		goto close1;
+
+	ret = fstat(fd1, &st1);
+	if (ret)
+		goto close2;
+	ret = fstat(fd2, &st2);
+	if (ret)
+		goto close2;
+
+	if (st1.st_size != st2.st_size)
+		goto close2;
+
+	map1 = mmap(NULL, st1.st_size, PROT_READ, MAP_PRIVATE, fd1, 0);
+	if (map1 == MAP_FAILED)
+		goto close2;
+
+	map2 = mmap(NULL, st2.st_size, PROT_READ, MAP_PRIVATE, fd2, 0);
+	if (map2 == MAP_FAILED)
+		goto close2;
+
+	if (bcmp(map1, map2, st1.st_size))
+		goto close2;
+
+	ret = true;
+close2:
+	close(fd2);
+close1:
+	close(fd1);
+
+	return ret;
+}
+
 /*
  * Create the parent directory of the given path.
  *
@@ -888,6 +935,13 @@  int conf_write(const char *name)
 	fclose(out);
 
 	if (*tmpname) {
+		if (is_same(name, tmpname)) {
+			conf_message("No change to %s", name);
+			unlink(tmpname);
+			sym_set_change_count(0);
+			return 0;
+		}
+
 		snprintf(oldname, sizeof(oldname), "%s.old", name);
 		rename(name, oldname);
 		if (rename(tmpname, name))