Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: fix EXTRAVERSION regex #85724

Closed
wants to merge 1 commit into from
Closed

Conversation

arnopo
Copy link
Collaborator

@arnopo arnopo commented Feb 13, 2025

Use '\' instead of '' for the regex argument described in cmake specification:
The quoted argument "\(\a\+b\)" specifies a regex that matches the exact string (a+b). Each \ is parsed in a quoted argument as just , so the regex itself is actually (\a+\b).

This fixes build error reported in OpenAMP CI

CMake Error at zephyrproject/zephyr/cmake/modules/version.cmake:66 (string):
Syntax error in cmake code at

zephyrproject/zephyr/cmake/modules/version.cmake:66

when parsing string

EXTRAVERSION = ([a-z0-9.-]*)

Invalid escape sequence .
Call Stack (most recent call first):
zephyrproject/zephyr/share/zephyr-package/cmake/ZephyrConfigVersion.cmake:59 (include)
cmake/syscheck.cmake:8 (find_package)
CMakeLists.txt:17 (include)

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though the final - doesn't need escaping, but does no harm neither.

@@ -63,7 +63,7 @@ foreach(type file IN ZIP_LISTS VERSION_TYPE VERSION_FILE)
string(REGEX MATCH "VERSION_TWEAK = ([0-9]*)" _ ${ver})
set(${type}_VERSION_TWEAK ${CMAKE_MATCH_1})

string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\.\-]*)" _ ${ver})
string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\\.\\-]*)" _ ${ver})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a small observation.
When - is placed at the end of matching brackets, [, ], then it doesn't need to be escaped.

Ref:

To match a literal - using brackets, make it the first or the last character, e.g., [+*/-] matches basic mathematical operators.

Suggested change
string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\\.\\-]*)" _ ${ver})
string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\\.-]*)" _ ${ver})

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of \\ actually matches a literal \, resulting in \ to become a valid char in the VERSION file.

For example using:

# VERSION File content
...
EXTRAVERSION = 4.5\-5

results in:

Loading Zephyr default modules (Zephyr repository).
...
-- Zephyr version: 4.0.99-4.5\-5 (/projects/github/review/zephyr)

which again causes issues later when Zephyr's VERSION is processed using \.

@tejlmand tejlmand added the DNM This PR should not be merged (Do Not Merge) label Feb 13, 2025
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of \\. (and \\-) actually means the regex matches a literal \, which is not what we want.

The reason for the error:

CMake Error at zephyrproject/zephyr/cmake/modules/version.cmake:66 (string):
Syntax error in cmake code at

zephyrproject/zephyr/cmake/modules/version.cmake:66

when parsing string

EXTRAVERSION = ([a-z0-9.-]*)

Invalid escape sequence .

is due to CMP0053 using old policy.
The use of old policy is due to the use of CMake minimum version 3.0.2 here:
https://github.com/OpenAMP/libmetal/blob/465fcf018c760785fc04fcd90a72b17ab9a8a58d/CMakeLists.txt#L1

Please note that Zephyr requires CMake version >=3.20.

This error caused by old CMake version required can also be verified with the following CMake snippet:

# test.cmake
cmake_minimum_required(VERSION 3.20)

string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\.-]*)" _ "EXTRAVERSION = 4.5-5")
message("Match: ${CMAKE_MATCH_1}")

prints:

$ cmake -P test.cmake
Match: 4.5-5

but:

# test.cmake
cmake_minimum_required(VERSION 3.0.2)

string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\.-]*)" _ "EXTRAVERSION = 4.5-5")
message("Match: ${CMAKE_MATCH_1}")

prints:

$ cmake -P test.cmake
CMake Error at test.cmake:4 (string):
  Syntax error in cmake code at

    /tmp/cmake/regex.cmake:4

  when parsing string

    EXTRAVERSION = ([a-z0-9\.-]*)

  Invalid escape sequence \.

Same error is seen when setting CMP0053 policy to old.

@@ -63,7 +63,7 @@ foreach(type file IN ZIP_LISTS VERSION_TYPE VERSION_FILE)
string(REGEX MATCH "VERSION_TWEAK = ([0-9]*)" _ ${ver})
set(${type}_VERSION_TWEAK ${CMAKE_MATCH_1})

string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\.\-]*)" _ ${ver})
string(REGEX MATCH "EXTRAVERSION = ([a-z0-9\\.\\-]*)" _ ${ver})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of \\ actually matches a literal \, resulting in \ to become a valid char in the VERSION file.

For example using:

# VERSION File content
...
EXTRAVERSION = 4.5\-5

results in:

Loading Zephyr default modules (Zephyr repository).
...
-- Zephyr version: 4.0.99-4.5\-5 (/projects/github/review/zephyr)

which again causes issues later when Zephyr's VERSION is processed using \.

Use '\\' instead of '\' for the regex argument'.', as described in
cmake specification:
The quoted argument "\\(\\a\\+b\\)" specifies a regex that matches
the exact string (a+b). Each \\ is parsed in a quoted argument as just \,
so the regex itself is actually \(\a\+\b\).

Remove \ for -, as it is not needed when placed at the end of matching
brackets.

This fixes build error reported in OpenAMP CI

CMake Error at zephyrproject/zephyr/cmake/modules/version.cmake:66
(string):
 Syntax error in cmake code at

 zephyrproject/zephyr/cmake/modules/version.cmake:66

 when parsing string

  EXTRAVERSION = ([a-z0-9\.\-]*)

 Invalid escape sequence \.
Call Stack (most recent call first):
 share/zephyr-package/cmake/ZephyrConfigVersion.cmake:59
 cmake/syscheck.cmake:8 (find_package)
 CMakeLists.txt:17 (include)


Signed-off-by: Arnaud Pouliquen <[email protected]>
@tejlmand tejlmand removed the DNM This PR should not be merged (Do Not Merge) label Feb 13, 2025
@arnopo
Copy link
Collaborator Author

arnopo commented Feb 13, 2025

Thanks @tejlmand for your deep analysis!

I will fix that in OpenAMP.

I will close this one except if you want that i sent the non mandatory fix for the '-'

@arnopo arnopo closed this Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System size: XS A PR changing only a single line of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants