[RFC] How to sync headers for firmware and driver

[jajanusz]

Headers from <firmware>/src/include/uapi/ipc and<kernel>/include/sound/sof have to be synced. Now we just copy them , it works, but requires attention and work from developers, also is error-prone.

The best would be to have some git submodule with headers shared by both repos, however we cannot do that in driver repo (we can pretty much just add c code and kconfigs there).

So if we'd like to deduplicate headers they'd have to be in SW repo only and FW should have it as submodule or some kind of substep in buildsystem (path to kernel repo in cmake option or checkout like cmocka for unit tests). However the smallest checkout you can do for topic/sof-dev with --depth 1 is still almost 200mb, it's a lot just for headers and git processing itself takes some time also. With git --filter maybe we would be able to just get include subfolder, but this feature in git is since September 2018 and is not even in GitHub.

IMHO deduplication of headers is troublesome and not worth the effort.

What should we do? - I think it'd be good to just make current process better. With the help of CI we can automate cloning / checks. We need at least some kind of tagging in PRs / commit messages for CI. So by default CI would check if headers are the same in PR and HEAD of <linux>/topic/sof-dev, but there should be also way to let CI know that this PR is intended for other revision in headers like [linux <hash>] in PR summary / commit message (we will need anyway something like that for automated testing of specific fw + sw pairs).

For automatic copying - I'm not sure how much github api can, but maybe for each change of these headers in 1 repo the CI user can automatically create PR for another one ?

[jajanusz]

@plbossart @lgirdwood @mmaka1 @xiulipan What do you think?

[lgirdwood]

Problem is we have multiple gatekeepers and differences to comment style and headers between them, so doing a regular diff or copy wont always work. However, we can have a script that can check them and highlight any meaningful differences i.e. we run them through the GCC pre-processor which will strip all comments and resolve all macros. This will be able to give us a meaning full diff, it could also be used alongside git to highlight any commits that are unbalanced.

[xiulipan]

@jajanusz A good idea to use CI to track changes on both. Some issues for the sync work I have done for the API headers:

1. Different folder structure. ex. <uapi/eq.h>(kernel) and <uapi/user/eq.h> (sof)
2. variable type mapping ex. u32 & uint32_t
3. merge time for the different PRs

[jajanusz]

@xiulipan 1&2 are easy to solve if you are going to normalize everything with gcc preprocessor before doing comparison (as @lgirdwood suggested). If you want to do some logical instead of textual comparsion, we can use some other tools or even write something with clang (but they are just simple headers, so I think it's not worth it).

1. Different folder structure. ex. <uapi/eq.h>(kernel) and <uapi/user/eq.h> (sof)

Just do something like ln -s <firmware>/src/include/uapi/ipc <kernel>/include/sound/sof and ln -s <firmware>/src/include/uapi/user <kernel>/include/uapi/sound/sof, or copy headers in the similar way. Look at it as path normalization (to fw or sw structure).

2. variable type mapping ex. u32 & uint32_t

I didn't findy any u32 in mentioned headers, but even if that's the problem then you can create simple compatibility header that is included by preprocessor, with defines like #define uint32_t u32, or just replace them with some script if you need something that can actually create AST (or understand code for some reason).

3. merge time for the different PRs

I'm not sure what you mean but I guess you meant that there is no atomic operation to do same commit/PR on 2 branches. That's why we need some kind of tagging. We can say that SW is consumer/user of the FW, so I think that changes should go to FW first and then checking if SW is using correct headers should be within SW's CI (+ any tools to make it more convenient).

[lgirdwood]

@xiulipan I dont want to see CI flagging up ABI header file deltas. This will be common and a normal development flow as FW and kernel have different repos (kernel will lag further when upstream). They key is to manage the delta and some simple tools will help here.

[xiulipan]

@lgirdwood Got it. So could we add a new tag as ABI for the track of the PR? That we can highlight ourselves to make sure we have some test and prepare on both SW and FW.

[lgirdwood]

@xiulipan no, as tagging is easy to forget and anyway the semantic versioning will mean FW/driver will still work at the oldest ABI feature level. The best thing is for maintainers to refuse to merge until PR's are pending for both FW + kernel on any ABI update (policy now started).

We do need the tools though to validate and bisect when/where/who to manage the differences when the kernel driver is upstream.

[xiulipan]

@lrgirdwo That would be a good start! Thanks

[wenqingfu]

Is the conclusion to turn this into a script CI runs on every PR? @jajanusz @xiulipan who will create the script for "meaning full diff"?

[lgirdwood]

@wenqingfu no CI script, since it will routinely fail for valid merges due to a race between kernel and firmware ABI updates. @xiulipan should be working on a script that provides a meaningful diff that can bisect the kernel/FW to show when differences were applied, the commit hash, author etc. This will allow us to track any delta.

[jajanusz]

@lgirdwood @xiulipan Any conclusions? IMO Now when we have 1 folder what is for kernel and you can literally just get files from that folder to 1 folder in kernel and ipc folder to another, it's easy to have bot/CI that takes care of keeping kernel in sync.

FW is provider of headers, so you just check if something got merged to the master and then do automatically PR in kernel repo, or just notify maintainers to do this.

In files content you need only to change:

1. License to dual bsd/gpl
2. Include guards
3. Include paths

That's all, even simple text processor can do this.

[lgirdwood]

@jajanusz we can only track delta (after running pre-processor) and then bisect any commits that are missing on on side. But in general that dashboard will show status for each PR (and I dont merge until dashboard is aligned for both kernel and FW).