[PATCH] tools: Remove #include <version.h>

[PATCH] tools: Remove #include <version.h>

[Pali Rohár]

Header file version.h includes also autogenerated file timestamp.h which
is recompiled on every time when SOURCE_DATE_EPOCH change.

Tools do not use build time therefore they do not have to include
timestamp.h file.

This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
changes.

Signed-off-by: Pali Roh?r <pali@kernel.org>
---
 tools/dumpimage.c  | 2 +-
 tools/mkenvimage.c | 2 +-
 tools/mkimage.c    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/dumpimage.c b/tools/dumpimage.c
index e5481435a7..54c2517c9e 100644
--- a/tools/dumpimage.c
+++ b/tools/dumpimage.c
@@ -7,7 +7,7 @@
 
 #include "dumpimage.h"
 #include <image.h>
-#include <version.h>
+#include "generated/version_autogenerated.h"
 
 static void usage(void);
 
diff --git a/tools/mkenvimage.c b/tools/mkenvimage.c
index b05f83415f..6482ca6ee9 100644
--- a/tools/mkenvimage.c
+++ b/tools/mkenvimage.c
@@ -23,7 +23,7 @@
 
 #include "compiler.h"
 #include <u-boot/crc.h>
-#include <version.h>
+#include "generated/version_autogenerated.h"
 
 #define CRC_SIZE sizeof(uint32_t)
 
diff --git a/tools/mkimage.c b/tools/mkimage.c
index 68d5206cb4..d5e274c504 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -11,7 +11,7 @@
 #include "mkimage.h"
 #include "imximage.h"
 #include <image.h>
-#include <version.h>
+#include "generated/version_autogenerated.h"
 
 static void copy_file(int, const char *, int);
 
-- 
2.20.1

[PATCH] tools: Remove #include <version.h>

[Simon Glass]

On Wed, 27 Jan 2021 at 08:34, Pali Roh?r <pali@kernel.org> wrote:
>
> Header file version.h includes also autogenerated file timestamp.h which
> is recompiled on every time when SOURCE_DATE_EPOCH change.
>
> Tools do not use build time therefore they do not have to include
> timestamp.h file.
>
> This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
> changes.
>
> Signed-off-by: Pali Roh?r <pali@kernel.org>
> ---
>  tools/dumpimage.c  | 2 +-
>  tools/mkenvimage.c | 2 +-
>  tools/mkimage.c    | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH] tools: Remove #include <version.h>

[Tom Rini]

On Wed, Jan 27, 2021 at 04:34:24PM +0100, Pali Roh?r wrote:

> Header file version.h includes also autogenerated file timestamp.h which
> is recompiled on every time when SOURCE_DATE_EPOCH change.
> 
> Tools do not use build time therefore they do not have to include
> timestamp.h file.
> 
> This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
> changes.
> 
> Signed-off-by: Pali Roh?r <pali@kernel.org>
> ---
>  tools/dumpimage.c  | 2 +-
>  tools/mkenvimage.c | 2 +-
>  tools/mkimage.c    | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/dumpimage.c b/tools/dumpimage.c
> index e5481435a7..54c2517c9e 100644
> --- a/tools/dumpimage.c
> +++ b/tools/dumpimage.c
> @@ -7,7 +7,7 @@
>  
>  #include "dumpimage.h"
>  #include <image.h>
> -#include <version.h>
> +#include "generated/version_autogenerated.h"
>  
>  static void usage(void);
>  

I don't know if I really like this approach.  It also seems inconsistent
as we have for example tools/fit-image.c that has content from
generated/version_autogenerated.h but wasn't changed by this patch (nor
the rest of the series).

I think a bit more invasive approach is required here.  I like the
<version_string.h> approach you used elsewhere, and as best I can see,
the only place U_BOOT_VERSION_STRING, which is where we have the
timestamp itself, is used is cmd/version.c (and defined in version.h).
We should isolate the file that has the date such that it's only
in one file and referenced in one file.  This means probably removing
the version_string global from arch/{powerpc,m68k} and dropping __weak
from cmd/version.c and seeing what happens, as well.

Do you see where I'm thinking with this, or do I need to try and explain
a bit more?  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/20210201/85338a69/attachment.sig>

[PATCH] tools: Remove #include <version.h>

[Pali Rohár]

On Monday 01 February 2021 16:20:18 Tom Rini wrote:
> On Wed, Jan 27, 2021 at 04:34:24PM +0100, Pali Roh?r wrote:
> 
> > Header file version.h includes also autogenerated file timestamp.h which
> > is recompiled on every time when SOURCE_DATE_EPOCH change.
> > 
> > Tools do not use build time therefore they do not have to include
> > timestamp.h file.
> > 
> > This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
> > changes.
> > 
> > Signed-off-by: Pali Roh?r <pali@kernel.org>
> > ---
> >  tools/dumpimage.c  | 2 +-
> >  tools/mkenvimage.c | 2 +-
> >  tools/mkimage.c    | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/dumpimage.c b/tools/dumpimage.c
> > index e5481435a7..54c2517c9e 100644
> > --- a/tools/dumpimage.c
> > +++ b/tools/dumpimage.c
> > @@ -7,7 +7,7 @@
> >  
> >  #include "dumpimage.h"
> >  #include <image.h>
> > -#include <version.h>
> > +#include "generated/version_autogenerated.h"
> >  
> >  static void usage(void);
> >  
> 
> I don't know if I really like this approach.  It also seems inconsistent
> as we have for example tools/fit-image.c that has content from
> generated/version_autogenerated.h but wasn't changed by this patch (nor
> the rest of the series).
> 
> I think a bit more invasive approach is required here.  I like the
> <version_string.h> approach you used elsewhere, and as best I can see,
> the only place U_BOOT_VERSION_STRING, which is where we have the
> timestamp itself, is used is cmd/version.c (and defined in version.h).
> We should isolate the file that has the date such that it's only
> in one file and referenced in one file.  This means probably removing
> the version_string global from arch/{powerpc,m68k} and dropping __weak
> from cmd/version.c and seeing what happens, as well.
> 
> Do you see where I'm thinking with this, or do I need to try and explain
> a bit more?  Thanks!

Hello Tom! I think I understood what you mean. I would need to look at
all places where version strings and timestamps in any forms are used as
this approach would need some other cleanup...

> 
> -- 
> Tom

[PATCH] tools: Remove #include <version.h>

[Pali Rohár]

On Monday 01 February 2021 16:20:18 Tom Rini wrote:
> On Wed, Jan 27, 2021 at 04:34:24PM +0100, Pali Roh?r wrote:
> 
> > Header file version.h includes also autogenerated file timestamp.h which
> > is recompiled on every time when SOURCE_DATE_EPOCH change.
> > 
> > Tools do not use build time therefore they do not have to include
> > timestamp.h file.
> > 
> > This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
> > changes.
> > 
> > Signed-off-by: Pali Roh?r <pali@kernel.org>
> > ---
> >  tools/dumpimage.c  | 2 +-
> >  tools/mkenvimage.c | 2 +-
> >  tools/mkimage.c    | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/dumpimage.c b/tools/dumpimage.c
> > index e5481435a7..54c2517c9e 100644
> > --- a/tools/dumpimage.c
> > +++ b/tools/dumpimage.c
> > @@ -7,7 +7,7 @@
> >  
> >  #include "dumpimage.h"
> >  #include <image.h>
> > -#include <version.h>
> > +#include "generated/version_autogenerated.h"
> >  
> >  static void usage(void);
> >  
> 
> I don't know if I really like this approach.  It also seems inconsistent
> as we have for example tools/fit-image.c that has content from
> generated/version_autogenerated.h but wasn't changed by this patch (nor
> the rest of the series).
> 
> I think a bit more invasive approach is required here.  I like the
> <version_string.h> approach you used elsewhere, and as best I can see,
> the only place U_BOOT_VERSION_STRING, which is where we have the
> timestamp itself, is used is cmd/version.c (and defined in version.h).
> We should isolate the file that has the date such that it's only
> in one file and referenced in one file.  This means probably removing
> the version_string global from arch/{powerpc,m68k} and dropping __weak
> from cmd/version.c and seeing what happens, as well.

Hello! I have looked at dropping this weak property from version_string
but seem it is not so easy... powerpc platforms have following code:

file arch/powerpc/cpu/mpc85xx/start.S:

       .globl  _start
_start:
       .long   0x27051956              /* U-BOOT Magic Number */
       .globl  version_string
version_string:
       .ascii U_BOOT_VERSION_STRING, "\0"

I do not know how it is used, but for me it looks like that on these
platforms is somehow null-term U-Boot version string important to be put
after some magic number at specific location (start of the binary?).

How could we handle this situation? I do not have any of those powerpc
platforms and I do not know any details about them.

If we cannot remove version string from this location, it is possible to
tweak linker to put at this location version string from compiled
cmd/version.o object file (at the time when building final u-boot
binary)?

CCing powerpc maintainers.

> Do you see where I'm thinking with this, or do I need to try and explain
> a bit more?  Thanks!
> 
> -- 
> Tom

[PATCH] tools: Remove #include <version.h>

[Pali Rohár]

On Wednesday 03 February 2021 12:10:13 Pali Roh?r wrote:
> On Monday 01 February 2021 16:20:18 Tom Rini wrote:
> > On Wed, Jan 27, 2021 at 04:34:24PM +0100, Pali Roh?r wrote:
> > 
> > > Header file version.h includes also autogenerated file timestamp.h which
> > > is recompiled on every time when SOURCE_DATE_EPOCH change.
> > > 
> > > Tools do not use build time therefore they do not have to include
> > > timestamp.h file.
> > > 
> > > This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
> > > changes.
> > > 
> > > Signed-off-by: Pali Roh?r <pali@kernel.org>
> > > ---
> > >  tools/dumpimage.c  | 2 +-
> > >  tools/mkenvimage.c | 2 +-
> > >  tools/mkimage.c    | 2 +-
> > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/dumpimage.c b/tools/dumpimage.c
> > > index e5481435a7..54c2517c9e 100644
> > > --- a/tools/dumpimage.c
> > > +++ b/tools/dumpimage.c
> > > @@ -7,7 +7,7 @@
> > >  
> > >  #include "dumpimage.h"
> > >  #include <image.h>
> > > -#include <version.h>
> > > +#include "generated/version_autogenerated.h"
> > >  
> > >  static void usage(void);
> > >  
> > 
> > I don't know if I really like this approach.  It also seems inconsistent
> > as we have for example tools/fit-image.c that has content from
> > generated/version_autogenerated.h but wasn't changed by this patch (nor
> > the rest of the series).
> > 
> > I think a bit more invasive approach is required here.  I like the
> > <version_string.h> approach you used elsewhere, and as best I can see,
> > the only place U_BOOT_VERSION_STRING, which is where we have the
> > timestamp itself, is used is cmd/version.c (and defined in version.h).
> > We should isolate the file that has the date such that it's only
> > in one file and referenced in one file.  This means probably removing
> > the version_string global from arch/{powerpc,m68k} and dropping __weak
> > from cmd/version.c and seeing what happens, as well.
> 
> Hello! I have looked at dropping this weak property from version_string
> but seem it is not so easy... powerpc platforms have following code:
> 
> file arch/powerpc/cpu/mpc85xx/start.S:
> 
>        .globl  _start
> _start:
>        .long   0x27051956              /* U-BOOT Magic Number */
>        .globl  version_string
> version_string:
>        .ascii U_BOOT_VERSION_STRING, "\0"
> 
> I do not know how it is used, but for me it looks like that on these
> platforms is somehow null-term U-Boot version string important to be put
> after some magic number at specific location (start of the binary?).
> 
> How could we handle this situation? I do not have any of those powerpc
> platforms and I do not know any details about them.
> 
> If we cannot remove version string from this location, it is possible to
> tweak linker to put at this location version string from compiled
> cmd/version.o object file (at the time when building final u-boot
> binary)?
> 
> CCing powerpc maintainers.

Hello! Do you have any details? Can be version_string at beginning
dropped? Or it is required?

> 
> > Do you see where I'm thinking with this, or do I need to try and explain
> > a bit more?  Thanks!
> > 
> > -- 
> > Tom
> 
> 

[PATCH] tools: Remove #include <version.h>

[Tom Rini]

On Thu, Feb 25, 2021 at 02:05:54AM +0100, Pali Roh?r wrote:
> On Wednesday 03 February 2021 12:10:13 Pali Roh?r wrote:
> > On Monday 01 February 2021 16:20:18 Tom Rini wrote:
> > > On Wed, Jan 27, 2021 at 04:34:24PM +0100, Pali Roh?r wrote:
> > > 
> > > > Header file version.h includes also autogenerated file timestamp.h which
> > > > is recompiled on every time when SOURCE_DATE_EPOCH change.
> > > > 
> > > > Tools do not use build time therefore they do not have to include
> > > > timestamp.h file.
> > > > 
> > > > This change prevents recompiling tools every time when SOURCE_DATE_EPOCH
> > > > changes.
> > > > 
> > > > Signed-off-by: Pali Roh?r <pali@kernel.org>
> > > > ---
> > > >  tools/dumpimage.c  | 2 +-
> > > >  tools/mkenvimage.c | 2 +-
> > > >  tools/mkimage.c    | 2 +-
> > > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/tools/dumpimage.c b/tools/dumpimage.c
> > > > index e5481435a7..54c2517c9e 100644
> > > > --- a/tools/dumpimage.c
> > > > +++ b/tools/dumpimage.c
> > > > @@ -7,7 +7,7 @@
> > > >  
> > > >  #include "dumpimage.h"
> > > >  #include <image.h>
> > > > -#include <version.h>
> > > > +#include "generated/version_autogenerated.h"
> > > >  
> > > >  static void usage(void);
> > > >  
> > > 
> > > I don't know if I really like this approach.  It also seems inconsistent
> > > as we have for example tools/fit-image.c that has content from
> > > generated/version_autogenerated.h but wasn't changed by this patch (nor
> > > the rest of the series).
> > > 
> > > I think a bit more invasive approach is required here.  I like the
> > > <version_string.h> approach you used elsewhere, and as best I can see,
> > > the only place U_BOOT_VERSION_STRING, which is where we have the
> > > timestamp itself, is used is cmd/version.c (and defined in version.h).
> > > We should isolate the file that has the date such that it's only
> > > in one file and referenced in one file.  This means probably removing
> > > the version_string global from arch/{powerpc,m68k} and dropping __weak
> > > from cmd/version.c and seeing what happens, as well.
> > 
> > Hello! I have looked at dropping this weak property from version_string
> > but seem it is not so easy... powerpc platforms have following code:
> > 
> > file arch/powerpc/cpu/mpc85xx/start.S:
> > 
> >        .globl  _start
> > _start:
> >        .long   0x27051956              /* U-BOOT Magic Number */
> >        .globl  version_string
> > version_string:
> >        .ascii U_BOOT_VERSION_STRING, "\0"
> > 
> > I do not know how it is used, but for me it looks like that on these
> > platforms is somehow null-term U-Boot version string important to be put
> > after some magic number at specific location (start of the binary?).
> > 
> > How could we handle this situation? I do not have any of those powerpc
> > platforms and I do not know any details about them.
> > 
> > If we cannot remove version string from this location, it is possible to
> > tweak linker to put at this location version string from compiled
> > cmd/version.o object file (at the time when building final u-boot
> > binary)?
> > 
> > CCing powerpc maintainers.
> 
> Hello! Do you have any details? Can be version_string at beginning
> dropped? Or it is required?

Should probably just go with trying linker games at this point, 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/20210225/e9e84b06/attachment.sig>