zephyr: namespace the generated version.h - take 1

[ycsin]

Zephyr's build time generated version.h is now in the zephyr to provide proper namespace, update the includes of this module accordingly.

See zephyrproject-rtos/zephyr#63973

[sofci]

Can one of the admins verify this patch?

reply test this please to run this test once

[kv2019i]

Change looks good, but I wonder @ycsin if we could introduce this incrementally? We have Zephyr use SOF as a submodule, but we also have SOF mainline using Zephyr as a module (sof/west.yml), so we have dependencies both ways, so changes like these are not easy to get through.

[ycsin]

Change looks good, but I wonder @ycsin if we could introduce this incrementally? We have Zephyr use SOF as a submodule, but we also have SOF mainline using Zephyr as a module (sof/west.yml), so we have dependencies both ways, so changes like these are not easy to get through.

There will be a new compatibility Kconfig that allows the use of legacy path in the Zephyr PR, does that address your concern?

[kv2019i]

@ycsin wrote:

There will be a new compatibility Kconfig that allows the use of legacy path in the Zephyr PR, does that address your concern?

But there's a chichen and egg problem. Most of our CI fails for this PR as the Zephyr version does not have zephyr/version.h.

Hmm, possible options:

• add a ifdef on Zephyr version to this PR and include "version.h" if old Zephyr and "zephyr/version.h" if new Zephyr. then CI should be good and we can merge this PR in SOF repo
• merge the new interface in Zephyr mainline and we merge this PR when we updated Zephyr version to sof/west.yml

[ycsin]

• add a ifdef on Zephyr version to this PR and include "version.h" if old Zephyr and "zephyr/version.h" if new Zephyr. then CI should be good and we can merge this PR in SOF repo

This won't work as the version macros require the header, another chicken & egg

• merge the new interface in Zephyr mainline and we merge this PR when we updated Zephyr version to sof/west.yml

Yet another 🐔 & 🥚 issue, the process to update the header paths in Zephyr is to update the paths in the affected modules first 😅

The CONFIG_COMPAT_INCLUDES Kconfig might be useful in this case, let's see..

[lyakh]

why are you removing kernel.h?

[ycsin]

oops, copy paste error

[ycsin]

fixed

[lyakh]

Can one of the admins verify this patch?

reply test this please to run this test once

test this please

[ycsin]

force pushed to amend author info

[kv2019i]

Thanks @ycsin ! This looks better now. Our CI tests that take the Zephyr main fail -- not sure why yet, I thought the version.h location has not changed.

[kv2019i]

Aa, this was just merged to main:

commit 702399080e94131c124d8c5428e6508e8c8d0bb1
Author: Fabio Baltieri <[email protected]>
Date:   Wed Nov 8 10:05:50 2023 +0000

    Kconfig: drop COMPAT_INCLUDES

Explains the fail in https://github.com/thesofproject/sof/actions/runs/6810140232/job/18519160165?pr=8459

[ycsin]

Aa, this was just merged to main:

commit 702399080e94131c124d8c5428e6508e8c8d0bb1
Author: Fabio Baltieri <[email protected]>
Date:   Wed Nov 8 10:05:50 2023 +0000

    Kconfig: drop COMPAT_INCLUDES

Explains the fail in https://github.com/thesofproject/sof/actions/runs/6810140232/job/18519160165?pr=8459

@fabiobaltieri I was going to use that Kconfig here for compatibility, should I create that Kconfig in sof instead?

EDIT: Added a Kconfig in the prj folder

[kv2019i]

Thanks @ycsin , this works!

[ycsin]

force pushed to add the license identifier to the newly added Kconfig file

[lgirdwood]

@marc-hb good for you ?

[ycsin]

not sure what's wrong with the Kconfig file, any idea?

[marc-hb]

Aa, this was just merged to main:
commit 702399080e94131c

Thanks @ycsin for fixing the build but please add references to the corresponding Zephyr changes in the commit message, that's the least the commit message requires.

Yet another 🐔 & 🥚 issue, the process to update the header paths in Zephyr is to update the paths in the affected modules first 😅

It should be easy enough to solve this sort of problem with a branch. We don't care if SOF CI does not build on a temporary branch.
Please don't add a new Kconfig just to solve a temporary chicken-and-egg problem, that's really not what Kconfig is for.

@marc-hb good for you ?

I'm afraid I wasn't copied on this.

[marc-hb]

Now I found the giant

• build: namespace generated headers with zephyr/ zephyrproject-rtos/zephyr#63973

... which finally gave me some better understanding. Please refer to it in this commit message, no one should have to search for this.

I don't see any zephyr/west.yml change in that 63973 (yet?) but I assume it is why you're submitting this PR 8459 to the sof/main branch, correct?

But zephyr/west.yml does not have to point at the SOF main branch:

https://docs.zephyrproject.org/latest/develop/modules.html#allowed-practices

Upstream changes brought in by performing a Git merge of the intended upstream branch (e.g. main branch, latest release branch, etc.)

Emphasis mine.

I also found zephyrproject-rtos#31 by sheer chance.
I disagree with @dbaluta there, I think PR 31 looked like a great way to solve the two-ways dependency/chicken-and-egg problem. Then we merge PR 31 into sof/main at the same time we uprev sof/west.yml past zephyr PR 8459 and problem solved.

It is of course preferred to minimize differences with upstream but there are plenty of exceptions and I think this is one of them. See other exceptions there:
https://github.com/zephyrproject-rtos/sof/pulls?q=is%3Apr+is%3Amerged

This PR 17 exception is funny enough!

• include: add "zephyr/" include prefix zephyrproject-rtos/sof#17

If zephyr/sof PR 31 is not possible for some reason, then @kv2019i can you please create some new transition branch https://github.com/thesofproject/sof/commits/version-h-migration or with some better name?

This is a pure branching problem, let's please not add unnecessary and temporary Kconfig complexity to solve it. Git should be enough.

[kv2019i]

@marc-hb Hmm, I don't fully get why this PR is such a big problem. But zephyrproject-rtos#31 is ok to me as well. Based on the comments, @dbaluta , this is probably ok for you if we sign up to cherry-pick when we uprev to newer Zephyr...?

[marc-hb]

@marc-hb Hmm, I don't fully get why this PR is such a big problem

It's a problem because when the build breaks (and it will for some people, this is a backwards incompatible change), the answer from 1st level support (me among others) should be as simple as "run west update" and walk away. It should not involve inspecting every user's .config file or any other unnecessary Kconfig acrobatics. That's not what Kconfig is designed for and the extra complexity is not needed here, west is enough. Let's keep it simple with everyone and everything on the same configuration.

[kv2019i]

@marc-hb Still not fully convinced as the Kconfig settings come from version-control as well, so west update will have similar effect. Also zephyrproject-rtos#31 will create a similar flag day and once the change is merged to Zephyr, you cannot go forward/backward unless you cherry-pick/drop the needed patch to SOF.

Oh well, I'm good with zephyrproject-rtos#31 approach, can we proceed with that @ycsin ?

[kv2019i]

Removing from v2.8 milestone as this is completely tied to the Zephyr upstream change and we'll have to make the change in tandem anyways. Whether that happens before or after v2.8 branch, has no impact on the release.

[ycsin]

can we proceed with that @ycsin ?

I can create another PR for that, but the CI in that PR is going to fail as the change requires changes made to the Zephyr upstream

EDIT: Created #8502

[marc-hb]

as the Kconfig settings come from version-control as well, so west update will have similar effect

west update will have the same effect only when you keep the Kconfig defaults, but then why is there a new Kconfig if the assumption is that everyone keeps the default Kconfig value?

I can imagine how a new Kconfig could help with some corner cases, but I prefer not to. Let's keep it simple with less flexibility and less complexity.

EDIT: Created #8502

I don't think anyone asked for #8502. I think we were all asking to just re-open zephyrproject-rtos#31 and that's all.

[marc-hb]

I don't understand why you just pushed this, this is confusing.

Please use zephyrproject-rtos#34 (which replaced 31 for some unknown reason) to resolve compilation issues entirely on the https://github.com/zephyrproject-rtos side, NOT involving https://github.com/thesofproject/ yet. Only after everything is merged and passing on the https://github.com/zephyrproject-rtos side, then we will look at the thesofproject side. Backwards-incompatible changes have always been (successfully) deployed like this.

[ycsin]

Sorry, I probably got confused myself 😅