RFC: include `version.h` in the build system #60651

ycsin · 2023-07-20T17:31:52Z

After working on #59981 & quite a bit of adaptation work to support multiple Zephyr versions that involve breaking changes, i.e. #51217 & #45417, it becomes clear that having access to the Zephyr kernel version can be handy:

to get the Zephyr kernel version, (almost) everywhere in a Zephyr application, and
to be able to the conditionally compile based on the kernel version

This PR demonstrates two ways to achieve this:

The first commit simply includes version.h in the zephyr/kernel.h - so any application that includes the kernel header will have access to its version
The second commit includes the version.h as part of the compile command just like the autoconf.h - so any application built out of Zephyr build system will have access to the kernel version, just like the Kconfigs.

The 2nd commit might be a better way, as it allows all Zephyr compiled application to access the kernel version & it reduces the chance of namespace conflict.

Would love to have some comments on this.

Include the MAKE generated version header file so that any Zephyr application that include the kernel header will know its version without including additional header. Signed-off-by: Yong Cong Sin <[email protected]>

Set compiler specific macro inclusion of the version.h just like the autoconf.h, so that the kernel version is accessible everywhere. Signed-off-by: Yong Cong Sin <[email protected]>

No need to include version.h manually in files. Signed-off-by: Yong Cong Sin <[email protected]>

cfriedt · 2023-07-20T17:40:13Z

I would support option 2, preferably.

Definitely will become relevant when we have people looking at migrating from LTSv2 to LTSv3 (or any other recent release, for that matter), cc @kartben

And as @ycsin mentions, people will want to run test suites, test harnesses with boards, etc, to ensure that tests are passing on old version and new version before making the leap.

So we have C/C++ source conditional compilation. What about CMake conditional compilation?

E.g. currently we can access Kconfig constants within CMakeLists.txt files. It would be nice to incorporate Zephyr kernel version too.

Thoughts, @tejlmand?

nordicjm · 2023-07-21T06:52:05Z

CMakeLists.txt

+# @Intent: Set compiler specific macro inclusion of VERSION_H
+zephyr_compile_options("SHELL: $<TARGET_PROPERTY:compiler,imacros> ${VERSION_H}")


OK with change except for this, should remain an include file which users #include if they need it. Having it in kernel.h I am undecided upon as it seems likely that some codebase or library somewhere will have a conflicting define

This line is the essence of the second way. Do you think name spacing the macros in the version.h (i.e. ZEPHYR_*) would help with mitigating the potential conflict?

I don't like this.

Code should include what it needs, so if the code requires version.h, then it should include version.h.
I don't see why version.h is more special than kernel.h, errno.h, or string.h which are includes you find almost everywhere.

nordicjm · 2023-07-21T06:53:42Z

E.g. currently we can access Kconfig constants within CMakeLists.txt files. It would be nice to incorporate Zephyr kernel version too.

You can access the version from cmake already:

target_sources(app PRIVATE src/main.c)
message("Version: ${KERNEL_VERSION_MAJOR}.${KERNEL_VERSION_MINOR}.${KERNEL_PATCHLEVEL}")

->

Version: 3.4.99
-- Configuring done (3.2s)
-- Generating done (0.0s)

Just a sample. Signed-off-by: Yong Cong Sin <[email protected]>

MaureenHelm · 2023-09-21T15:10:32Z

Please rebase and address the failed CI checks

tejlmand

I cannot approve this approach.

Both proposed methods will cause almost all files to depend upon version.h meaning it will severely impact incremental builds for no good reason.

See also: #40167 and #42527

tejlmand · 2023-10-06T07:36:18Z

cmake/modules/version.cmake

  unset(${type}_VERSION_WITHOUT_TWEAK)
 endforeach()
+
+set(VERSION_H ${PROJECT_BINARY_DIR}/include/generated/version.h)


this is not the right location for this piece of information.

This file is used for CMake and is used both for Zephyr's kernel version, Application version, but can also be used by downstream users.

The intention of this CMake code is to parse a VERSION file (Zephyr, app, downstream project, etc.) and define corresponding CMake variables.

The VERSION_H is a Zephyr code specific header and thus belongs more correctly where it is used today.

Notice also that this call:

zephyr/CMakeLists.txt

Lines 558 to 563 in 657462d

COMMAND ${CMAKE_COMMAND} -DZEPHYR_BASE=${ZEPHYR_BASE}

-DOUT_FILE=${VERSION_H}

-DVERSION_TYPE=KERNEL

-DVERSION_FILE=${ZEPHYR_BASE}/VERSION

${build_version_argument}

-P ${ZEPHYR_BASE}/cmake/gen_version_h.cmake

with your proposal both feeds in VERSION_H as the OUT_FILE argument, but that the script gen_version_h.cmake actually calls on to version.cmake where you just made that piece on info directly available.

zephyr/cmake/gen_version_h.cmake

Line 32 in 9e8d609

include(${ZEPHYR_BASE}/cmake/modules/version.cmake)

Suggested change

set(VERSION_H ${PROJECT_BINARY_DIR}/include/generated/version.h)

tejlmand · 2023-10-06T07:47:15Z

CMakeLists.txt

+# @Intent: Set compiler specific macro inclusion of VERSION_H
+zephyr_compile_options("SHELL: $<TARGET_PROPERTY:compiler,imacros> ${VERSION_H}")


I don't like this.

Code should include what it needs, so if the code requires version.h, then it should include version.h.
I don't see why version.h is more special than kernel.h, errno.h, or string.h which are includes you find almost everywhere.

tejlmand · 2023-10-06T10:32:13Z

include/zephyr/kernel.h

 #define ZEPHYR_INCLUDE_KERNEL_H_

 #if !defined(_ASMLANGUAGE)
+#include "version.h" /* generated by MAKE, at compile time */


I'm not fond of this.

kernel.h itself doesn't require anything / provide anything related to version, so I fail to see why it should suddenly start to include a header so that other parts of code can save a single include line.

Doing this will have a severe impact on incremental builds.
On incremental builds, then only code that contains actual changes (or headers they include) will rebuild if there is a need.
But including version.h in kernel.h means that everyone will depend on version.h, and thus commiting a foo.txt, unrelated to other files will cause everything to rebuild.

Today:

$ cmake -GNinja -DBOARD=nrf52840dk_nrf52840 -Bbuild samples/hello_world $ ninja -C build $ ninja: Entering directory `build' ... [134/134] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used FLASH: 18152 B 1 MB 1.73% RAM: 5580 B 256 KB 2.13% IDT_LIST: 0 GB 2 KB 0.00% $ ninja -C build ninja: Entering directory `build' ninja: no work to do. $ touch foo.txt; git add foo.txt; git commit -s -m"File added" $ ninja -C build ninja: Entering directory `build' [1/9] Generating include/generated/version.h -- Zephyr version: 3.5.0-rc1 (/projects/github/ncs/zephyr), build: v3.5.0-rc1-45-gb59168b4833f [9/9] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used FLASH: 18152 B 1 MB 1.73% RAM: 5580 B 256 KB 2.13% IDT_LIST: 0 GB 2 KB 0.00%

Note, that the new file causes a new commit, which generates a new version.h, but only files actually using version.h needs to be recompiled, and thus linked.

Just 9 build step.

If kernel.h includes version.h, then almost everything will rebuild.
Result of doing the #include "version.h" change in my repo,

$ ninja -C build ninja: Entering directory `build' ninja: no work to do. $ touch foo.txt; git add foo.txt; git commit -s -m"File added" $ ninja -C build; ninja: Entering directory `build' [1/89] Generating include/generated/version.h -- Zephyr version: 3.5.0-rc1 (/projects/github/ncs/zephyr), build: v3.5.0-rc1-45-gdf07a813c711 [89/89] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used FLASH: 18152 B 1 MB 1.73% RAM: 5580 B 256 KB 2.13% IDT_LIST: 0 GB 2 KB 0.00%

cfriedt · 2023-10-12T01:59:39Z

I cannot approve this approach.

Both proposed methods will cause almost all files to depend upon version.h meaning it will severely impact incremental builds for no good reason.

See also: #40167 and #42527

@tejlmand has a point

Unneeded rebuilds are no good.

I think the biggest concern was that <version.h> could easily conflict with some other header and it would be nice to keep it namespaced for Zephyr.

I wonder if we could deprecate the old <version.h> in favour of <zephyr/version.h>. Keep the copy around for a couple of releases and then drop the old one.

That might be the second best solution and only requires a bit of patience.

But it would definitely be good to get in before LTSv3.

tejlmand · 2023-10-12T14:15:10Z

I wonder if we could deprecate the old <version.h> in favour of <zephyr/version.h>. Keep the copy around for a couple of releases and then drop the old one.

that sounds like a good suggestion.

ycsin · 2023-10-17T02:22:06Z

@tejlmand Q: wouldn't it make sense for the software being compiled to know the version of the kernel? To me the software should be able to 'just know' the kernel version without including anything, just like the autoconf.h. How about introducing another Kconfigs like CONFIG_KERNEL_MAJOR & CONFIG_KERNEL_MINOR?

nordicjm · 2023-10-17T06:36:49Z

@tejlmand Q: wouldn't it make sense for the software being compiled to know the version of the kernel? To me the software should be able to 'just know' the kernel version without including anything, just like the autoconf.h. How about introducing another Kconfigs like CONFIG_KERNEL_MAJOR & CONFIG_KERNEL_MINOR?

No, Kconfig symbols would stay set to the version that was used to configure it, that would mean if you configured a project with 3.5, they would show 3.5, if you then just switched your zephyr version without deleting the build to version 3.0 and built it again, it would still show 3.5, likewise if you changed to 4.1 and did the same, it would still show 3.5

ycsin added 3 commits July 20, 2023 23:55

kernel: include version.h

cd506ed

Include the MAKE generated version header file so that any Zephyr application that include the kernel header will know its version without including additional header. Signed-off-by: Yong Cong Sin <[email protected]>

build: Set compiler specific macro inclusion of VERSION_H

657462d

Set compiler specific macro inclusion of the version.h just like the autoconf.h, so that the kernel version is accessible everywhere. Signed-off-by: Yong Cong Sin <[email protected]>

zephyr: remove version.h

584c6c0

No need to include version.h manually in files. Signed-off-by: Yong Cong Sin <[email protected]>

ycsin requested review from andyross, carlescufi, cfriedt, cvinayak, dcpleung, de-nordic, jakub-uC, kruithofa, nashif, nordic-krch, nordicjm, rlubos, tejlmand and thoh-ot as code owners July 20, 2023 17:31

zephyrbot added area: Kernel area: OpenThread area: Bluetooth Controller area: Build System area: POSIX POSIX API Library area: Networking area: mcumgr area: Bluetooth area: Shell Shell subsystem labels Jul 20, 2023

zephyrbot requested a review from ceolin July 20, 2023 17:32

zephyrbot assigned nordicjm Jul 20, 2023

ycsin requested review from MaureenHelm and galak as code owners July 20, 2023 17:37

ycsin removed request for de-nordic and kruithofa July 20, 2023 17:37

nordicjm reviewed Jul 21, 2023

View reviewed changes

samples: an example of supporting multiple version

56e84ee

Just a sample. Signed-off-by: Yong Cong Sin <[email protected]>

ycsin force-pushed the pr/include-version branch from 0cc95a4 to 56e84ee Compare July 27, 2023 02:36

tejlmand requested changes Oct 6, 2023

View reviewed changes

ycsin closed this Oct 23, 2023

ycsin deleted the pr/include-version branch November 14, 2023 03:43

		# @Intent: Set compiler specific macro inclusion of VERSION_H
		zephyr_compile_options("SHELL: $<TARGET_PROPERTY:compiler,imacros> ${VERSION_H}")

	COMMAND ${CMAKE_COMMAND} -DZEPHYR_BASE=${ZEPHYR_BASE}
	-DOUT_FILE=${VERSION_H}
	-DVERSION_TYPE=KERNEL
	-DVERSION_FILE=${ZEPHYR_BASE}/VERSION
	${build_version_argument}
	-P ${ZEPHYR_BASE}/cmake/gen_version_h.cmake

RFC: include version.h in the build system #60651

RFC: include version.h in the build system #60651

Uh oh!

Conversation

ycsin commented Jul 20, 2023

Uh oh!

cfriedt commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordicjm Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

ycsin Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

nordicjm commented Jul 21, 2023

Uh oh!

MaureenHelm commented Sep 21, 2023

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt commented Oct 12, 2023

Uh oh!

tejlmand commented Oct 12, 2023

Uh oh!

ycsin commented Oct 17, 2023

Uh oh!

nordicjm commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RFC: include `version.h` in the build system #60651

RFC: include `version.h` in the build system #60651

cfriedt commented Jul 20, 2023 •

edited

Loading

ycsin Jul 21, 2023 •

edited

Loading

tejlmand Oct 6, 2023 •

edited

Loading