Change in flashrom[master]: util/getversion,meson: Add script to allow version info with Meson

[Nico Huber (Code Review)]

Hello Mario Limonciello, Jacob Garber,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/flashrom/+/35561

to review the following change.

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

util/getversion,meson: Add script to allow version info with Meson

Add `util/getversion.sh` that retrieves version information from a
`versioninfo.inc` (what we use for releases) if present or uses
`util/getrevision.sh` if not.

Let Meson use it for flashrom's version. It seems Meson doesn't
generate the manual page at all, so the `--man-date` command is
currently unused.

Change-Id: I401e5638509c4a573bc0cb17ebc5fa76df9700b5
Signed-off-by: Nico Huber 
---
M meson.build
A util/getversion.sh
2 files changed, 72 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/35561/1

diff --git a/meson.build b/meson.build
index e1b6c16..b475f64 100644
--- a/meson.build
+++ b/meson.build
@@ -1,5 +1,5 @@
 project('flashromutils', 'c',
-  version : '1.0',
+  version : run_command('util/getversion.sh', '-v').stdout().strip(),
   license : 'GPL-2.0+',
   meson_version : '>=0.47.0',
   default_options : ['warning_level=2', 'c_std=c99'],
diff --git a/util/getversion.sh b/util/getversion.sh
new file mode 100755
index 0000000..d3810d2
--- /dev/null
+++ b/util/getversion.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+#
+# This file is part of the flashrom project.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+
+version() {
+	if [ -r versioninfo.inc ]; then
+		v=$(sed -n 's/^VERSION = //p' versioninfo.inc)
+	else
+		v=$($(dirname ${0})/getrevision.sh --revision)
+		if [ $? -ne 0 ]; then
+			v='unknown'
+		fi
+	fi
+
+	echo ${v}
+}
+
+mandate() {
+	if [ -r versioninfo.inc ]; then
+		d=$(sed -n 's/^MAN_DATE = //p' versioninfo.inc)
+	else
+		d=$($(dirname ${0})/getrevision.sh --date flashrom.8.tmpl)
+		if [ $? -ne 0 ]; then
+			d='unknown'
+		fi
+	fi
+
+	echo ${d}
+}
+
+show_help() {
+	echo "Usage:
+	${0} <command>
+
+Commands
+    -h or --help
+        this message
+    -v or --version
+        return current/release flashrom version
+    -m or --man-date
+        return current/release date of the manual page
+"
+}
+
+if [ $# -ne 1 ]; then
+	show_help
+	echo "Error: Only exactly one command allowed.">&2
+	exit 1
+fi
+
+case $1 in
+	-h|--help)	show_help;;
+	-m|--man-date)	mandate;;
+	-v|--version)	version;;
+	*)
+		show_help
+		echo "Error: Invalid option: $1"
+		exit 1
+		;;
+esac

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:

This is certainly functional, but I think that the version strings spit out don't make sense for libflashrom.  They include letters and are not monotonically increasing, even at tagged releases.  So I think some sed magic is going to be needed to make it useful.

Here was what I got out of a test run in pkg-config: "Version: v1.1-rc1-85-ge094fb4"

Quoting https://people.freedesktop.org/~dbn/pkg-config-guide.html:
> The Name, Description and URL fields are purely informational and should be easy to fill in. 
> The Version field is a bit trickier to ensure that it is usable by consumers of the data. 
> pkg-config uses the algorithm from RPM for version comparisons. This works best with a 
> dotted decimal number such as 1.2.3 since letters can cause unexpected results. The number 
> should be monotonically increasing and be as specific as possible in describing the library. > Usually it's sufficient to use the package's version number here since it's easy for 
> consumers to track.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:
> This is certainly functional, but I think that the version strings spit out don't make sense for libflashrom.  They include letters and are not monotonically increasing, even at tagged releases.  So I think some sed magic is going to be needed to make it useful.

Understood, and I agree. But it's unrelated to this patch. This patch
changes the version of `project('flashromutils'...` it doesn't say
libflashrom. If libflashrom/pkg-config needs special handling, that
should be treated separately (I don't know how). If need be, I would
agree to remove the v prefix from releases. But versions between the
releases just don't have numbers...

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:
> Patch Set 1:
> > This is certainly functional, but I think that the version strings spit out don't make sense for libflashrom.  They include letters and are not monotonically increasing, even at tagged releases.  So I think some sed magic is going to be needed to make it useful.
>
> Understood, and I agree. But it's unrelated to this patch. This patch
> changes the version of `project('flashromutils'...` it doesn't say
> libflashrom. If libflashrom/pkg-config needs special handling, that
> should be treated separately (I don't know how). If need be, I would
> agree to remove the v prefix from releases. But versions between the
> releases just don't have numbers...

Well further down in meson.build it pulls that number for pkg-config.
> version : meson.project_version(),

So I think some sanitization needs to happen rather than just picking that up.  I would propose something like dropping the prefix and if there is a '-' sed everything from that into a .999

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > This is certainly functional, but I think that the version strings spit out don't make sense for libflashrom.  They include letters and are not monotonically increasing, even at tagged releases.  So I think some sed magic is going to be needed to make it useful.
> >
> > Understood, and I agree. But it's unrelated to this patch. This patch
> > changes the version of `project('flashromutils'...` it doesn't say
> > libflashrom. If libflashrom/pkg-config needs special handling, that
> > should be treated separately (I don't know how). If need be, I would
> > agree to remove the v prefix from releases. But versions between the
> > releases just don't have numbers...
>
> Well further down in meson.build it pulls that number for pkg-config.
> > version : meson.project_version(),
>
> So I think some sanitization needs to happen rather than just picking that up.  I would propose something like dropping the prefix and if there is a '-' sed everything from that into a .999

Here is the code to make that happen:

```
diff --git a/meson.build b/meson.build
index b475f64..0af6047 100644
--- a/meson.build
+++ b/meson.build
@@ -349,10 +349,19 @@ flashrom = shared_library(
   link_depends : mapfile,
 )

+version = meson.project_version()
+if version.startswith('v')
+  version = version.split('v')[1]
+endif
+if version.contains ('-')
+  version = version.split('-')[0] + '.999'
+endif
+message(version)
+
 pkgg = import('pkgconfig')
 pkgg.generate(
   libraries : flashrom,
-  version : meson.project_version(),
+  version : version,
   name : 'libflashrom',
   filebase : 'libflashrom',
   description : 'libflashrom',

```

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:
> > > > This is certainly functional, but I think that the version strings spit out don't make sense for libflashrom.  They include letters and are not monotonically increasing, even at tagged releases.  So I think some sed magic is going to be needed to make it useful.
> > >
> > > Understood, and I agree. But it's unrelated to this patch. This patch
> > > changes the version of `project('flashromutils'...` it doesn't say
> > > libflashrom. If libflashrom/pkg-config needs special handling, that
> > > should be treated separately (I don't know how). If need be, I would
> > > agree to remove the v prefix from releases. But versions between the
> > > releases just don't have numbers...
> >
> > Well further down in meson.build it pulls that number for pkg-config.
> > > version : meson.project_version(),
> >
> > So I think some sanitization needs to happen rather than just picking that up.  I would propose something like dropping the prefix and if there is a '-' sed everything from that into a .999

.999 would imply it's later than any point release that might
still happen in the future. How about no suffix at all? that
would somehow imply there is nothing new in the library since
the last release that should be expected in the future (matches
flashrom's development model).
> +version = meson.project_version()
> +if version.startswith('v')
> version = version.split('v')[1]
>
> +endif

We also have to expect and remove 'p' (see [1]). Using
split() looks odd, but I don't know how to do it better.
> +if version.contains ('-')
> version = version.split('-')[0] + '.999'

I would prefer to drop the .999

[1] https://flashrom.org/Development_Guidelines

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:
> Patch Set 1:
> > > > > This is certainly functional, but I think that the version strings spit out don't make sense for libflashrom.  They include letters and are not monotonically increasing, even at tagged releases.  So I think some sed magic is going to be needed to make it useful.
> > > >
> > > > Understood, and I agree. But it's unrelated to this patch. This patch
> > > > changes the version of `project('flashromutils'...` it doesn't say
> > > > libflashrom. If libflashrom/pkg-config needs special handling, that
> > > > should be treated separately (I don't know how). If need be, I would
> > > > agree to remove the v prefix from releases. But versions between the
> > > > releases just don't have numbers...
> > >
> > > Well further down in meson.build it pulls that number for pkg-config.
> > > > version : meson.project_version(),
> > >
> > > So I think some sanitization needs to happen rather than just picking that up.  I would propose something like dropping the prefix and if there is a '-' sed everything from that into a .999
>
> .999 would imply it's later than any point release that might
> still happen in the future. How about no suffix at all? that
> would somehow imply there is nothing new in the library since
> the last release that should be expected in the future (matches
> flashrom's development model).

Yes no suffix is fine to me considering that.
> > +version = meson.project_version()
> > +if version.startswith('v')
> > version = version.split('v')[1]
> >
> > +endif
>
> We also have to expect and remove 'p' (see [1]). Using
> split() looks odd, but I don't know how to do it better.

Yeah I guess 'p' needs to be filtered too then.
> > +if version.contains ('-')
> > version = version.split('-')[0] + '.999'
>
> I would prefer to drop the .999
>
> [1] https://flashrom.org/Development_Guidelines

Sure seems fine by me.  Go head and add this to your CL as you see fit.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 1:
> Sure seems fine by me.  Go head and add this to your CL as you see fit.

IMHO, it's an orthogonal change and belongs into it's own commit.
Also, it was your idea, I don't see why you shouldn't have the
honor :)

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 2:

Can you rebase this to fix the merge conflict and then can someone review it?

[Nico Huber (Code Review)]

Hello Mario Limonciello, Jacob Garber, Richard Hughes, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/35561

to look at the new patch set (#3).

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

util/getversion,meson: Add script to allow version info with Meson

Add `util/getversion.sh` that retrieves version information from a
`versioninfo.inc` (what we use for releases) if present or uses
`util/getrevision.sh` if not.

Let Meson use it for flashrom's version. It seems Meson doesn't
generate the manual page at all, so the `--man-date` command is
currently unused.

Change-Id: I401e5638509c4a573bc0cb17ebc5fa76df9700b5
Signed-off-by: Nico Huber 
---
M meson.build
A util/getversion.sh
2 files changed, 72 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/35561/3

[Mario Limonciello (Code Review)]

Mario Limonciello has uploaded a new patch set (#4) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

util/getversion,meson: Add script to allow version info with Meson

Add `util/getversion.sh` that retrieves version information from a
`versioninfo.inc` (what we use for releases) if present or uses
`util/getrevision.sh` if not.

Let Meson use it for flashrom's version. It seems Meson doesn't
generate the manual page at all, so the `--man-date` command is
currently unused.

Change-Id: I401e5638509c4a573bc0cb17ebc5fa76df9700b5
Signed-off-by: Nico Huber 
---
M meson.build
A util/getversion.sh
2 files changed, 72 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/35561/4

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4: Code-Review+1

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4: Code-Review+1

Thanks for merging those other 2 Nico.  I guess this is the last one I know about before tagging a new release.

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4:
> Patch Set 4: Code-Review+1
>
> Thanks for merging those other 2 Nico.  I guess this is the last one I know about before tagging a new release.

Nico, can you merge this one too?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4: Code-Review+1
> >
> > Thanks for merging those other 2 Nico.  I guess this is the last one I know about before tagging a new release.
>
> Nico, can you merge this one too?

Still needs a +2 by somebody else.

[Richard Hughes (Code Review)]

Richard Hughes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4: Code-Review+1

I didn't know you could execute an external script for the version -- it's a good idea indeed to avoid the versions being out of sync. With this fwupd should work against upstream!

[Richard Hughes (Code Review)]

Richard Hughes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4:

If we can get this upstream soonish we can switch fwupd to using the upstream flashrom rather than my crazy fork with the meson changes. This means we can start compiling fwupd with flashrom enabled in distros, which I've got tons of people asking me about :)

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4: Code-Review+2

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4:

Nico, since David gave you +2 can you submit now?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/flashrom/+/35561/4/meson.build 
File meson.build:

https://review.coreboot.org/c/flashrom/+/35561/4/meson.build@2 
PS4, Line 2: run_command('util/getversion.sh', '-v').stdout().strip()
nice pipeline

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

util/getversion,meson: Add script to allow version info with Meson

Add `util/getversion.sh` that retrieves version information from a
`versioninfo.inc` (what we use for releases) if present or uses
`util/getrevision.sh` if not.

Let Meson use it for flashrom's version. It seems Meson doesn't
generate the manual page at all, so the `--man-date` command is
currently unused.

Change-Id: I401e5638509c4a573bc0cb17ebc5fa76df9700b5
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/35561
Tested-by: build bot (Jenkins) 
Reviewed-by: Mario Limonciello 
Reviewed-by: Richard Hughes 
Reviewed-by: David Hendricks 
Reviewed-by: Angel Pons 
---
M meson.build
A util/getversion.sh
2 files changed, 72 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  David Hendricks: Looks good to me, approved
  Angel Pons: Looks good to me, approved
  Mario Limonciello: Looks good to me, but someone else must approve
  Richard Hughes: Looks good to me, but someone else must approve

diff --git a/meson.build b/meson.build
index d1f663f..b63875e 100644
--- a/meson.build
+++ b/meson.build
@@ -1,5 +1,5 @@
 project('flashromutils', 'c',
-  version : '1.0',
+  version : run_command('util/getversion.sh', '-v').stdout().strip(),
   license : 'GPL-2.0',
   meson_version : '>=0.47.0',
   default_options : ['warning_level=2', 'c_std=c99'],
diff --git a/util/getversion.sh b/util/getversion.sh
new file mode 100755
index 0000000..d3810d2
--- /dev/null
+++ b/util/getversion.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+#
+# This file is part of the flashrom project.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+
+version() {
+	if [ -r versioninfo.inc ]; then
+		v=$(sed -n 's/^VERSION = //p' versioninfo.inc)
+	else
+		v=$($(dirname ${0})/getrevision.sh --revision)
+		if [ $? -ne 0 ]; then
+			v='unknown'
+		fi
+	fi
+
+	echo ${v}
+}
+
+mandate() {
+	if [ -r versioninfo.inc ]; then
+		d=$(sed -n 's/^MAN_DATE = //p' versioninfo.inc)
+	else
+		d=$($(dirname ${0})/getrevision.sh --date flashrom.8.tmpl)
+		if [ $? -ne 0 ]; then
+			d='unknown'
+		fi
+	fi
+
+	echo ${d}
+}
+
+show_help() {
+	echo "Usage:
+	${0} <command>
+
+Commands
+    -h or --help
+        this message
+    -v or --version
+        return current/release flashrom version
+    -m or --man-date
+        return current/release date of the manual page
+"
+}
+
+if [ $# -ne 1 ]; then
+	show_help
+	echo "Error: Only exactly one command allowed.">&2
+	exit 1
+fi
+
+case $1 in
+	-h|--help)	show_help;;
+	-m|--man-date)	mandate;;
+	-v|--version)	version;;
+	*)
+		show_help
+		echo "Error: Invalid option: $1"
+		exit 1
+		;;
+esac

[Mario Limonciello (Code Review)]

Mario Limonciello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35561 )

Change subject: util/getversion,meson: Add script to allow version info with Meson
......................................................................

Patch Set 6:

Thanks for submitting. Can we get a new release tagged now?