| Message ID | 1297503431-19435-1-git-send-email-wd@denx.de |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Sat, 12 Feb 2011 10:37:11 +0100 Wolfgang Denk <wd@denx.de> wrote: > - @( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \ > - '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp > + @( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \ > + printf '#define PLAIN_VERSION "%s%s"\n' \ > + "$(U_BOOT_VERSION)" "$${localvers}" ; \ > + printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \ > + "$(U_BOOT_VERSION)" "$${localvers}" ; \ > + ) > $@.tmp IMO, PLAIN_VERSION isn't descriptive enough (should really be called VERSION..?). How about going with something like: #define U_BOOT_STR "U-Boot" #define U_BOOT_VERSION U_BOOT_STR " %s%s"... and then > + case 'V': > + printf("mkimage version %s\n", PLAIN_VERSION); > + exit(EXIT_SUCCESS); &U_BOOT_VERSION[sizeof(U_BOOT_STR)] (the - 1 is not necessary since we want to include the ' ') this maintains consistency and the fact that the mkimage version is directly tied to it's parent project, U-Boot's, version number. Kim
Dear Kim Phillips, In message <20110212171349.f0f5d472.kim.phillips@freescale.com> you wrote: > > > - @( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \ > > - '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp > > + @( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \ > > + printf '#define PLAIN_VERSION "%s%s"\n' \ > > + "$(U_BOOT_VERSION)" "$${localvers}" ; \ > > + printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \ > > + "$(U_BOOT_VERSION)" "$${localvers}" ; \ > > + ) > $@.tmp > > IMO, PLAIN_VERSION isn't descriptive enough (should really be called > VERSION..?). How about going with something like: > > #define U_BOOT_STR "U-Boot" > #define U_BOOT_VERSION U_BOOT_STR " %s%s"... No - not unless you guarantee that this syntax is compatible with all assemblers that may be used to build U-Boot. > and then > > > + case 'V': > > + printf("mkimage version %s\n", PLAIN_VERSION); > > + exit(EXIT_SUCCESS); > > &U_BOOT_VERSION[sizeof(U_BOOT_STR)] > > (the - 1 is not necessary since we want to include the ' ') No again. This is, um, ugly, and completely unnecessary. Best regards, Wolfgang Denk
On Sun, 13 Feb 2011 00:35:08 +0100 Wolfgang Denk <wd@denx.de> wrote: > Dear Kim Phillips, > > In message <20110212171349.f0f5d472.kim.phillips@freescale.com> you wrote: > > > > > - @( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \ > > > - '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp > > > + @( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \ > > > + printf '#define PLAIN_VERSION "%s%s"\n' \ > > > + "$(U_BOOT_VERSION)" "$${localvers}" ; \ > > > + printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \ > > > + "$(U_BOOT_VERSION)" "$${localvers}" ; \ > > > + ) > $@.tmp > > > > IMO, PLAIN_VERSION isn't descriptive enough (should really be called > > VERSION..?). How about going with something like: > > > > #define U_BOOT_STR "U-Boot" > > #define U_BOOT_VERSION U_BOOT_STR " %s%s"... > > No - not unless you guarantee that this syntax is compatible with all > assemblers that may be used to build U-Boot. I cannot do that, but, ok, not such a big deal: /* prefix lengths must match */ #define U_BOOT_STR "U-Boot" #define U_BOOT_VERSION "U-Boot %s%s"... > > > + case 'V': > > > + printf("mkimage version %s\n", PLAIN_VERSION); > > > + exit(EXIT_SUCCESS); > > > > &U_BOOT_VERSION[sizeof(U_BOOT_STR)] > > > > (the - 1 is not necessary since we want to include the ' ') > > No again. This is, um, ugly, and completely unnecessary. that's a matter of personal taste, but IMO it's better than PLAIN_VERSION (what's that? - VERSION_NUMBERONLY would be way more descriptive). If it's the &..[..] that's not appealing, feel free to do as the '+ 7' code but as '+ sizeof(...)' (to maintain a not-so-completely unnecessary clarity & consistency..but that's my opinion). Kim
Dear Wolfgang Denk, In message <1297503431-19435-1-git-send-email-wd@denx.de> you wrote: > Signed-off-by: Wolfgang Denk <wd@denx.de> > --- > v2: fix missing argument to printf() call. > v3: explain the magic "+ 7" offset into the version string > v3: avoid offset into U_BOOT_VERSION string completely and define > a new PLAIN_VERSION variable instead; this has the benefit that it > can be used in other places as well where such version information > might be needed (fw_{set,print}env etc. comes to mind). > > Makefile | 8 ++++++-- > tools/mkimage.c | 6 ++++++ > 2 files changed, 12 insertions(+), 2 deletions(-) Applied. Best regards, Wolfgang Denk
diff --git a/Makefile b/Makefile index 05b404d..8721b59 100644 --- a/Makefile +++ b/Makefile @@ -414,8 +414,12 @@ $(U_BOOT_ONENAND): $(ONENAND_IPL) $(obj)u-boot.bin cat $(ONENAND_BIN) $(obj)u-boot.bin > $(obj)u-boot-onenand.bin $(VERSION_FILE): - @( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \ - '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp + @( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \ + printf '#define PLAIN_VERSION "%s%s"\n' \ + "$(U_BOOT_VERSION)" "$${localvers}" ; \ + printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \ + "$(U_BOOT_VERSION)" "$${localvers}" ; \ + ) > $@.tmp @( printf '#define CC_VERSION_STRING "%s"\n' \ '$(shell $(CC) --version | head -n 1)' )>> $@.tmp @( printf '#define LD_VERSION_STRING "%s"\n' \ diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..60f7263 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -23,6 +23,7 @@ #include "mkimage.h" #include <image.h> +#include <version.h> static void copy_file(int, const char *, int); static void usage(void); @@ -246,6 +247,9 @@ main (int argc, char **argv) case 'v': params.vflag++; break; + case 'V': + printf("mkimage version %s\n", PLAIN_VERSION); + exit(EXIT_SUCCESS); case 'x': params.xflag++; break; @@ -590,6 +594,8 @@ usage () params.cmdname); fprintf (stderr, " %s [-D dtc_options] -f fit-image.its fit-image\n", params.cmdname); + fprintf (stderr, " %s -V ==> print version information and exit\n", + params.cmdname); exit (EXIT_FAILURE); }
Signed-off-by: Wolfgang Denk <wd@denx.de> --- v2: fix missing argument to printf() call. v3: explain the magic "+ 7" offset into the version string v3: avoid offset into U_BOOT_VERSION string completely and define a new PLAIN_VERSION variable instead; this has the benefit that it can be used in other places as well where such version information might be needed (fw_{set,print}env etc. comes to mind). Makefile | 8 ++++++-- tools/mkimage.c | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-)