comm: make sure that our version check is reliable

[vincenzopalazzo]

Rework the logic of the version check used in the
database migration, and make sure
that it is full functional to avoid confusion
at release time.

Fixes #5870

Reported-by: @urza
Signed-off-by: Vincenzo Palazzo [email protected]

[SimonVrouwe]

Reported-by: @urza

What did he report?

[vincenzopalazzo]

What did he report?

that our version check did not work with the version release

[SimonVrouwe]

Just an idea, notice that there exist:

lightning/ccan/ccan/tal/str/str.h

Lines 180 to 185 in 3a39c63

	/**
	* tal_strreg - match/extract from a string via (extended) regular expressions.
	* @ctx: the context to tal from (often NULL)
	* @string: the string to try to match (can be take())
	* @regex: the regular expression to match (can be take())
	* ...: pointers to strings to allocate for subexpressions.

Which you could use to match the version string with a regex, maybe a release version should always match this "^v[0-9]+\.[0-9]+(\.[0-9]+)?$" ? (not tested)

[vincenzopalazzo]

Just an idea, notice that there exist:

great idea, I just think to search inside ccan for a function like that!

maybe a release version should always match this "^v[0-9]+.[0-9]+(.[0-9]+)?$" ? (not tested)

pushing the code and see if the tests works! thanks

[SimonVrouwe]

NACK 7c2e8c1

I think you missed the correction, the regex should be "^v[0-9]+\.[0-9]+(\.[0-9]+)?$", codes as:
char *regex = "^v[0-9]+\\.[0-9]+(\\.[0-9]+)?$"; as in below play example:

#include <stdio.h>
#include "ccan/tal/tal.h"
#include <ccan/tal/str/str.h>

int main(int argc, char *argv[])
{
	char *input, *match;
	char *regex = "^v[0-9]+\\.[0-9]+(\\.[0-9]+)?$";

	// Join args and trim trailing space.
	input = tal_strjoin(NULL, argv+1, " ", STR_NO_TRAIL);
	printf("input: \'%s\'\n", input);
	if (tal_strreg(NULL, input, regex, NULL, &match))
		printf("matches regex: \'%s\' \n", input, regex);
	else
		printf("not matching regex: \'%s\' \n", regex);
	return 0;
}

the major.minor.patch separator should always be a '.' (dot) and I don't see the need for forcing numbers to be two digits.

IMHO v23.1, v23.01 and v0.12.1 are all valid major release versions.

[vincenzopalazzo]

the major.minor.patch separator should always be a '.' (dot) and I don't see the need for forcing numbers to be two digits.

This is wrong, we are in year.month.patch, and without making things more difficult than they already are, we should accept just v23.01 and v23.01.1. As the following regex describe ^v\d{2}.\d{2}(.\d{1, 3})?$.

There is any spec like https://semver.org/ where I can understand that your option is the correct one regarding our versioning model?

[vincenzopalazzo]

@SimonVrouwe do you note any mistake in my code that causes the CI failure? regex is pretty unreadable, I'm tented to revert the regex usage and match just the length as original proposed

common/test/run-version > /dev/null
run-version: common/test/run-version.c:11: main: Assertion `cmp_release_version("v22.11")' failed.
Aborted (core dumped)
make: *** [Makefile:740: unittest/common/test/run-version] Error 134

[rustyrussell]

I like the tests, but the fix is actually to just change strcspn to strspn!

[rustyrussell]

Ack 20c081d

[rustyrussell]

Hmm, might want to make Changelog more explicit:

Changelog-Fixed: database: Correctly identity official release versions for database upgrade.

[vincenzopalazzo]

Hmm, might want to make Changelog more explicit:

Yep, thanks!

ACK 619746e

Just a commit format change

[SimonVrouwe]

There is any spec like https://semver.org/ where I can understand that your option is the correct one regarding our versioning model?

@vincenzopalazzo Again an incomprehensible sentence, is it a statement or a question? Please learn English.

From above link you provided:

No, “v1.2.3” is not a semantic version. However, prefixing a semantic version with a “v” is a common way (in English) to indicate it is a version number. Abbreviating “version” as “v” is often seen with version control.

and the paragraph Is there a suggested regular expression (RegEx) to check a SemVer string?, with an example showing that 23.01.0 is not a valid SemVer, but 23.1.0 is. But I guess I am being pedantic as c-lightning doesn't follow such rules.

[vincenzopalazzo]

@SimonVrouwe we are no longer using https://semver.org