PX4-Autopilot icon indicating copy to clipboard operation
PX4-Autopilot copied to clipboard

Git tag regex does not match error message

Open jorgenman opened this issue 3 years ago • 3 comments

Description

px_update_git_header.py checks the latest git tag to make sure it matches the approved format, and provides an error message if if doesn't:

The expected format is 'v<PX4 version>[-<custom version>]'
  <PX4 version>: v<major>.<minor>.<patch>[-rc<rc>|-beta<beta>|-alpha<alpha>|-dev]
  <custom version>: <major>.<minor>.<patch>[-rc<rc>|-beta<beta>|-alpha<alpha>|-dev]
Examples:
  v1.9.0-rc3 (preferred)
  v1.9.0-beta1
  v1.9.0-1.0.0
  v1.9.0-1.0.0-alpha2
See also https://dev.px4.io/master/en/setup/building_px4.html#firmware_version

From this message, it sounds like <PX4 version> and <custom version> can each have a suffix ([-rc<rc>|-beta<beta>|-alpha<alpha>|-dev]), but the actual regex does not allow a suffix on the <PX4 version> if there's a <custom version>.

Examples

The following git tags currently fail the check: v1.9.0-alpha2-1.0.0 v1.9.0-alpha2-1.0.0-beta1

Suggested fix

I'm happy to submit a PR, but I need to know what the desired behavior is. Is the current behavior correct (and the error message needs to be fixed)? Or, should the above examples fail (and the regex needs to be fixed)?

I feel like my above examples should be valid tags, and the regex should be fixed, but I'm not intimately familiar with exactly how the tag is used after this script.

jorgenman avatar Jul 07 '22 18:07 jorgenman

The parser currently only supports a rc/alpha/beta for the custom version if I remember correctly. Which means the regex needs to be fixed, but preferrably we extend the implementation. Here are the tests: https://github.com/PX4/PX4-Autopilot/blob/main/src/systemcmds/tests/test_versioning.cpp#L77=. And the implementation: https://github.com/PX4/PX4-Autopilot/blob/main/src/lib/version/version.c

bkueng avatar Jul 08 '22 07:07 bkueng

In the existing tests, it looks like the FIRMWARE_TYPE for the version number and vendor version number are expected to be the same. For example, with the string 0.45.99-1.2.3beta4, both the PX version number and the version number get an 80 at the end.

Based on this, I'm inclined to say that my requested example (v1.9.0-alpha2-1.0.0-beta1) shouldn't be supported. To support it, I think one of the following would have to happen, both of which seem problematic:

  • (bad) option 1: The firmware types in the version numbers still need to always be the same. So, one or the other always takes precedence, and the last byte of both version numbers uses that type, and the other is ignored.
  • (bad) option 2: Modify the implementation to allow different firmware types for the two version numbers. This would give different version numbers than the current implementation in some cases, which seems like a problem. For example, in v1.6.2-1.0.0-rc2, the PX type would be FIRMWARE_TYPE_RELEASE, instead of FIRMWARE_TYPE_RC as in the current implementation.

jorgenman avatar Sep 19 '22 03:09 jorgenman

Option 2 is to me the expected behavior and the current implementation is broken for that case. The existing behavior should be achieved with v1.6.2-rc2-1.0.0

bkueng avatar Sep 20 '22 05:09 bkueng