| Message ID | 20210127153424.4650-1-pali@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | tools: Remove #include <version.h> | expand |
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>
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!
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
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
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 > >
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.
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);
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(-)