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

Build Version detection not working when Zephyr Kernel is a Git Submodule #43503

Closed
ft opened this issue Mar 7, 2022 · 13 comments · Fixed by #43578
Closed

Build Version detection not working when Zephyr Kernel is a Git Submodule #43503

ft opened this issue Mar 7, 2022 · 13 comments · Fixed by #43578
Assignees
Labels
area: Build System bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@ft
Copy link
Contributor

ft commented Mar 7, 2022

Describe the bug

In 9170977 build-time version header generation was added. This uses

if(DEFINED BUILD_VERSION)
  set(build_version_argument "-DBUILD_VERSION=${BUILD_VERSION}")
elseif(EXISTS ${ZEPHYR_BASE}/.git/index)
  set(git_dependency ${ZEPHYR_BASE}/.git/index)
else()
  message(WARNING "ZEPHYR_BASE=${ZEPHYR_BASE} doesn't appear to be a git "
    "repository, please specify '-DBUILD_VERSION=<version>'")
endif()

When the repository is a git sub-module¹, the .git type is not a directory, but a regular file, that may look like this:

gitdir: ../../../.git/modules/firmware/zephyr/kernel

…which points to the module's actual git-database inside the super-project.

With that, the else() branch of the if statement is triggered, triggering the warning that's attached to it.

¹ https://git-scm.com/docs/git-submodule

To Reproduce

Steps to reproduce the behavior:

  1. Make a git-repository, with a blinky application inside.
  2. Add the zephyr-kernel repository as a sub-module.
  3. Build the project.
  4. Observe the described warning.

Expected behavior

The warning is not triggered.

Pick up the index file from the super-project's git-database connected to the kernel's sub-module and file it into git_dependency (I assume that's doing the trick; I didn't check what gen_version_h.cmake does; but the way it is used right now suggests it).

Impact

It looks like the zephyr/include/generated/version.h file still is generated correctly. At least initially, because the add_custom_command() call that generates the file is issued regardless of the tested conditions. The fact that git_dependency is not set correctly, will likely make this inconsistent. If it wouldn't, the added git_dependency in the current code would not serve any purpose.

Logs and console output

Don't have it available right now; this happened on a work-machine. Filing the issue from home.

Environment

  • Linux; but Windows should be affected too.
  • gnuarm on debian 10; but this is not toolchain dependent.
  • Zephyr v3.0.0; but the code in current main (08d98c2 at the time of writing) uses the same code.

Additional context

There is not much additional context, as the issue should affect everybody who uses the zephyr kernel from a git sub-module instead of a standalone clone.

@ft ft added the bug The issue is a bug, or the PR is fixing a bug label Mar 7, 2022
@stephanosio
Copy link
Member

Is there any reason why you are not using the west module? (see https://github.com/zephyrproject-rtos/example-application/blob/dfc6b88d25e2d738fe20f86e5e8ac40e0831cda9/west.yml#L13)

Adding the Zephyr repository as a Git submodule is not a normal workflow when it comes to Zephyr, and may even be said to be unsupported because there really is no reason to do so under normal circumstances.

cc @tejlmand

@ft
Copy link
Contributor Author

ft commented Mar 7, 2022

We are using git sub-modules to precisely define the versions of all included software in a larger system and use cmake directly instead of west. This works nicely with the non-zephyr parts of the software system we are building. I fail to see how this is abnormal, unless west was upgraded to a hard dependency at some point in time, that I must have missed.

If this use-case were to be declared unsupported, that would be rather sad to hear. FWIW, in the past such issues were addressed (i.e. #7207).

@stephanosio
Copy link
Member

I fail to see how this is abnormal, unless west was upgraded to a hard dependency at some point in time

Maybe @mbolivar-nordic can provide some insight.

We are using git sub-modules to precisely define the versions of all included software in a larger system and use cmake directly instead of west.

I assume you are referring to this:
https://github.com/ft/chip-remote/tree/master/firmware

If you have a look at the "example-application", it effectively achieves what you described above without using Git submodules.

If you do not like calling west build for whatever reason, you can still directly call CMake even when your repository is using the west modules (the only time you need to use west is when updating the west modules using west update).

@ft
Copy link
Contributor Author

ft commented Mar 7, 2022

I fail to see how this is abnormal, unless west was upgraded to a hard dependency at some point in time
Maybe @mbolivar-nordic can provide some insight.

The documentation still has its without-west¹ page, FWIW.

¹ https://docs.zephyrproject.org/latest/guides/west/without-west.html

We are using git sub-modules to precisely define the versions of all included software in a larger system and use cmake directly instead of west.

I assume you are referring to this: https://github.com/ft/chip-remote/tree/master/firmware

Well, this is an open-source project of mine, that uses the same idea to build its firmware code, as do the larger systems that are developed at work. It's quite a limited example, because there is nothing else except the firmware here.

If you have a look at the "example-application", it effectively achieves what you described above without using Git submodules.

If you do not like calling west build for whatever reason, you can still directly call CMake even when your repository is using the west modules (the only time you need to use west is when updating the west modules using west update).

There are two things that I would like to do: a) Use git to specify all software versions, not a separate format that is only used by zephyr; and b) Build all combinations of applications/boards/toolchains/build-configurations as declared in a top-level CMakeLists.txt as a subset of software to be produced by a cmake build. The latter is totally unrelated the issue, though, and could certainly by solved by means other than cmake.

As for the former, I understand that zephyr decided against direct use of git submodules for code organisation, and instead invest time into the development of west and I can understand the rationale. But that does not invalidate the use of them in situations in which they work perfectly well — and those are certainly numerous.

@mbolivar-nordic mbolivar-nordic added the priority: low Low impact/importance bug label Mar 8, 2022
@ft
Copy link
Contributor Author

ft commented Mar 8, 2022

I have a possible fix for this in ft@39d7c25.

Also, this thread made me read up on some of the newer things that west can do (it's been a while since I last looked). There's some interesting features in there. There's also some support for initialising submodules, so I guess it's not the worst idea in the world to keep supporting this. :)

WDYT?

@stephanosio
Copy link
Member

At the time of writing, we have not officially declared the west utility to be a requirement for using Zephyr; but, we are almost headed there (at least, for the module/component management).

Usages like this will likely become unsupported or unsustainable in the future and I would highly recommend that you use the west since installing it is as simple as running one or two commands and there is really nothing to lose by doing so.

Nevertheless, there is nothing wrong with submitting a fix for issues like this in the meantime.

@tejlmand
Copy link
Collaborator

tejlmand commented Mar 9, 2022

but, we are almost headed there

No, we have committed to keep west an optional tool.
Multiple users of Zephyr are having their own multi-repo management systems controlling and tracking their projects and we won't force those into west, so west will still be optional.

Usages like this will likely become unsupported or unsustainable in the future

No, the use-case that @ft is presenting is a very valid use case, and the fact that I didn't consider it when implementing #42527 is an oversight on my side.

@stephanosio
Copy link
Member

No, we have committed to keep west an optional tool.

While that may be the case on paper, we keep adding new features and schemes that make it harder to use Zephyr without west.

Unless you are into making your life harder for no good reason, I would strongly recommend using west.

@tejlmand
Copy link
Collaborator

tejlmand commented Mar 9, 2022

we keep adding new features and schemes that make it harder to use Zephyr without west.

not sure which features you are referring to and what exactly you mean by harder ?
But if we take an example like sysbuild, #40555, then of course using west build makes it easier to invoke sysbuild, but you can still invoke CMake directly for the same feature. Calling CMake instead of west build you may regard as harder (although I seldom use west build, mostly cmake directly).
But if a user / company opt-out of west, then they probably will create their own wrapper if they will benefit for sysbuild.

Features like west flash might not be important for users with boards that has multiple SoC or DSPs, where only a single SoC is running Zephyr. All the other SoCs / DSPs probably need custom flashing tools / scripts anyway.

In such cases, there can be good reasons for not using west.
But if Zephyr is the center of your product, then sure, west is the way to go.

@gmarull
Copy link
Member

gmarull commented Mar 9, 2022

No, we have committed to keep west an optional tool.

While that may be the case on paper, we keep adding new features and schemes that make it harder to use Zephyr without west.

Unless you are into making your life harder for no good reason, I would strongly recommend using west.

Strongly agree.

I think Zephyr lacks a proper guide on making Zephyr applications. We have a lot of details, but not a guide that says, "this is the recommended way to go for most use cases". Luckily, we now have example-application, which I think it helps a bit on this front. If every Zephyr user goes its own way, changes upstream will likely have an impact when upgrading Zephyr, leading to bad experience/Zephyr reputation. Another story is how Zephyr designs stuff internally, e.g., I agree it is good to decouple west/CMake.

@tejlmand
Copy link
Collaborator

this is the recommended way to go for most use cases

I agree that west is the recommended way, but there is a long way from having this as a recommended way, and then to state:

Adding the Zephyr repository as a Git submodule is not a normal workflow when it comes to Zephyr, and may even be said to be unsupported

Usages like this will likely become unsupported or unsustainable in the future

Highlighted words by me.
This is what I'm addressing, cause stating that such workflows is unsupported or will be unsupported is simply not true.
There are big users of Zephyr who are using other tools, mainly because Zephyr is just a subset (and thus not the center) of larger projects. Such projects can be using git submodules, google repo or any other tool.

So we should not start to give the impression that the use of Zephyr is locked to west.

@stephanosio
Copy link
Member

Sure, unsupported might not have been the best choice of words, but it already is and will likely become unsustainable.

stephanosio pushed a commit to ft/zephyr that referenced this issue Mar 11, 2022
In 9170977 build-time version header generation was added. The test
for .git assumes this file to be a directory. In the case of git
submodules, .git is a regular file that in its contents points to
the actual git database for the submodule. This is a way to have
symlink like behaviour even on file systems that do not support
themselves support symlinks.

This consults git as to what the correct git database directory is,
in case the .git file is indeed a regular file, and adjusts the
git_dependency variable accordingly.

Fixes zephyrproject-rtos#43503

Signed-off-by: Frank Terbeck <[email protected]>
mbolivar-nordic pushed a commit that referenced this issue Mar 15, 2022
In 9170977 build-time version header generation was added. The test
for .git assumes this file to be a directory. In the case of git
submodules, .git is a regular file that in its contents points to
the actual git database for the submodule. This is a way to have
symlink like behaviour even on file systems that do not support
themselves support symlinks.

This consults git as to what the correct git database directory is,
in case the .git file is indeed a regular file, and adjusts the
git_dependency variable accordingly.

Fixes #43503

Signed-off-by: Frank Terbeck <[email protected]>
@mbolivar-nordic
Copy link
Contributor

I want to pile on here for the record while I'm going over older issues and PRs today.

West is, was, and shall remain optional. However, not using west may be inconvenient. This was always the plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants