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

[FEATURE] remove SOF build time dependency to rimage headers #8178

Closed
kv2019i opened this issue Sep 7, 2023 · 18 comments
Closed

[FEATURE] remove SOF build time dependency to rimage headers #8178

kv2019i opened this issue Sep 7, 2023 · 18 comments
Assignees
Labels
enhancement New feature or request unclear Unclear enhancement request
Milestone

Comments

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 7, 2023

Is your feature request related to a problem? Please describe.
rimage was originally moved to a different repository as it is an independent tool from SOF.

With changes since SOF2.4, we have more and more direct dependencies from SOF to rimage. IPC4 protocol requires metadata that is defined in firmware manifest (data maintained in toml files, rimage encodes this to firmware binary file).
Most recently, loadable module support was added to SOF firmware and this code depends on firmware file structure definitions that are needed both by rimage and the new SOF module loader.

The build dependencies are causing issues to downstream SOF users (e.g. zephyrproject-rtos/zephyr#62262 ).

Having the rimage manifest changes (like thesofproject/rimage#177) not tested by full SOF CI, is causing operational issues (problems caused by rimage changes are detected too late).

Describe the solution you'd like
Merge rimage back to sof repository as it is now more integrate part of SOF.

Describe alternatives you've considered

  • alternative: duplicate manifest headers in SOF (these are part of the loadable modules ABI)
    - cons: does not help with SOF CI woes with rimage PRs

cc:

@kv2019i kv2019i added the enhancement New feature or request label Sep 7, 2023
@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 7, 2023

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 7, 2023

cc: @andyross , @dbaluta , @iuliana-prodan , @dcpleung

rimage was originally moved to a different repository as it is an independent tool from SOF.

Anyone remembers the rationale for a separate rimage repo? We don't want to go back and forth and back. I know booting Zephyr tests on Intel/NXP hardware is possible with rimage and without SOF. Could have this been a reason?

Having more git repos for no good reason makes everything more complicated. Among others and as mentioned above, it makes CI especially more complicated: how do you submit an rimage change (or Zephyr) to SOF testing? You can:

... BUT it's tedious and experience shows most people don't take the time.

EDIT: the complexity of splitting rimage was not missed in #2086 but it happened anyway.

https://en.wikipedia.org/wiki/Conway%27s_law aside (does not apply here), the ability to make arbitrary combinations is probably the only good reason to have multiple, independent repos. For instance we want the latest SOF-TEST branch to be able to test most SOF branches out of the box. We don't want to branch SOF-TEST every time that we branch SOF, that would be much more work. Obviously, you also don't want SOF-TEST code to change while bisecting SOF. So: a different repo for SOF-TEST. It's different for lower level CMocka unit tests which are in the same repo.

Similarly, we want the latest SOF docker image to be able to test any SOF branch, which shows sof/scripts/docker_build/ should be in a separate repo:

Back to rimage: do we need the ability to freely combine any rimage version with any SOF version? I guess not.

PS: https://en.wikipedia.org/wiki/Monorepo (@ Google, Facebook, Microsoft)

cc:

@plbossart
Copy link
Member

2019 change in 9495677

At some point there was a decision that tools don't belong in the SOF tree. That was after we merged them into SOF from https://github.com/thesofproject/soft to avoid the sort of problems we now have.

This clearly means we don't have a clear idea of what we are doing :-)

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 7, 2023

Below is a gitarcheology of the past moves with links to the corresponding discussions. I hope I did not miss anything.

The most relevant link is #2086 , this is where most of the rimage split discussion took place.

October 2016: creation of new sof-tools.git with rimage import as the first commit

February 2018: rimage is moved from sof-tools.git/rimage/ to sof.git/rimage/ to "remove a cyclic dependency between these two repos"

Git history is not transferred... yet! See next step.
The sof-tools.git/tools/rimage/keys/ subdirectory is copied but not removed and also left behind for some unknown reason.

November 2018: the rest of sof-tools.git follows and is also merged into sof.git to "reduce complexity".

This time the sof-tools git history is merged into sof.git thanks to @cujomalainey. This funnily enough resurrects the old sof-tools.git/rimage/ history in sof.git: https://github.com/thesofproject/sof/commits/72d797d9ddae7

sof-tools.git is frozen/archived.

--- SHORT-LIVED, SINGLE SOF REPO NIRVANA !? --

Nov 2019 - May 2020: rimage split from sof.git to a new rimage submodule (ick) for (at least) some Zephyr reason:

rimage history is not transferred.

This clearly means we don't have a clear idea of what we are doing :-)

In the links above I think I spotted some "plbossart" guy who already said that at the time.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 13, 2023

the ability to make arbitrary combinations is probably the only good reason to have multiple, independent repos.

In a meeting it was noted that rimage has recently gained more features which has made it more SOF-dependent than it was when it was split in May 2020.

So I ran some basic git log --oneline ... | wc commands on https://github.com/thesofproject/sof/commits/main/rimage and https://github.com/thesofproject/rimage/commits/main to quickly gauge the rimage activity. It has roughly doubled since January 2023:

Between June 2020 (right after it was split) and January 2023

  • 133 rimage commits in 30 months
  • submodule bumped 26 times in 30 months.

Since January 2023:

  • 81 commits in 9 months
  • submodule bumped 12 times in 9 months.

Of course these are just commit numbers, they don't directly measure dependencies.

@cujomalainey
Copy link
Member

Thanks for the history @marc-hb I agree, anytime we are expressing the same information across multiple repos we are in for a bad time. Now that the manifest contains data from the modules I think the split is not acceptable anymore. In reality I would rather the manifest derive from the C definition of the component so it's a single file but that is a different problem entirely.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 19, 2023

FYI @pjdobrowolski

@aiChaoSONG
Copy link
Collaborator

aiChaoSONG commented Sep 19, 2023

IMHO: rimage could be in a separate repo, or we could move it under sof/tools, just like other tools, but those toml files should stay with sof.

I have implemented the code to copy Intel IPC4 toml files to sof/config, split to platform related toml to sof/config/platform/, module related toml to sof/config/module/`, files are still named as PLATFORM.toml and update xtensa-build-zephyr.py build script.

Further split could be done for module toml, we could have one toml for each module, and only include the toml if it is enabled in kconfig. I could raise an RFC once #8212 got merged.

@abonislawski
Copy link
Member

IMHO: rimage could be in a separate repo, or we could move it under sof/tools, just like other tools, but those toml files should stay with sof.
Further split could be done for module toml, we could have one toml for each module, and only include the toml if it is enabled in kconfig.

Looks like a good idea, rimage repo + in SOF toml per module, just like all the rest module files in SOF, no need to maintain one giant toml in another repo

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 20, 2023

More .toml flexibility sounds good no matter what. So it's hopefully off-topic here.

EDIT: I take "off-topic" back :-(

@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 25, 2023

I'm tagging this as "unclear" as we don't have a consensus whether this is a problem to begin with.

@lgirdwood
Copy link
Member

@nashif do you need rimage moved into SOF with unified headers ?

@lgirdwood
Copy link
Member

@kv2019i @aiChaoSONG - let move rimage back into SOF and deprecate the rimage repo for v2.8.

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 2, 2023

The main question is whether we move rimage.git back into sof/tools/rimage (smooth option) or into sof/rimage (brutal option).

The brutal option is faster but it creates a "flag day" that can't be git bisected across and it will likely break other git commands for submodule users = all XTOS users + some Zephyr users.

The smoother, sof/tools/rimage option requires a transition period and a few scripting changes but it shouldn't break much. I think it's the safer choice.

cd sof/rimage

# We will need a ref to fetch and merge
git fetch thesofproject/
git checkout -t thesofproject/main

# Smooth option 
mkdir -p tools/rimage
find . -maxdepth 1 ! -name . ! -name .git ! -name tools -exec  git mv {} tools/rimage/ \;

# Brutal option
mkdir  rimage/
find . -maxdepth 1 ! -name . ! -name .git ! -name rimage -exec  git mv {} rimage/ \;

git commit -m 'move everything down to prepare git merge'

# cd back up to sof
cd ..

# Fetch rimage history into sof history
git remote add rimage-repo rimage
git fetch rimage-repo

git submodule deinit rimage || true
rm -rf ../sof/rimage # we already fetched it

git merge --allow-unrelated-histories -m 'Merging rimage history back into sof/tools/' rimage-repo/main

# Delete rimage module and re-add tomlc99 submodule
sed '/path.*tomlc99/ s#tomlc99#rimage/tomlc99#' tools/rimage/.gitmodules > .gitmodules 
git rm tools/rimage/.gitmodules
git commit -m 're-add tomlc99 submodule'

git submodule update --init

# Drop rimage in sof/west.yml
west update

(build)

TODO: re-enable rimage actions

@lgirdwood
Copy link
Member

@marc-hb pls go ahead and make the smooth option change, it seems you have most items worked out already. Thanks !

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 4, 2023

I'm on it, so far it seems easier than I thought it would be (famous last words). I will hopefully submit a PR this week.

@marc-hb marc-hb self-assigned this Oct 4, 2023
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
In preparation for changing it, see
thesofproject#8178

No functional change yet.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
In preparation for changing it, see
   thesofproject#8178

No functional change yet.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
In preparation for changing it, see
   thesofproject#8178

No functional change yet.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Preparation to merge back into https://github.com/thesofproject/sof/
repo, see thesofproject#8178

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
In preparation for changing it, see
   thesofproject#8178

No functional change yet.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
kv2019i pushed a commit that referenced this issue Oct 5, 2023
In preparation for changing it, see
#8178

No functional change yet.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
kv2019i pushed a commit that referenced this issue Oct 5, 2023
In preparation for changing it, see
   #8178

No functional change yet.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
… sof

Preparation to move everything back into the
https://github.com/thesofproject/sof/ repo, see
thesofproject#8178 for details.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 5, 2023
Move the full rimage git history back into the sof git repo.

Also list incoming tools/rimage/tomlc99 16000 gitlink in
sof/.gitmodules to avoid breaking all git submodule commands.

See thesofproject#8178 for details.
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 6, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 6, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 6, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 9, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 9, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this issue Oct 9, 2023
Switch away from the independent rimage submodule. Long story in
thesofproject#8178 and others.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator

marc-hb commented Oct 9, 2023

rimage transfer ready for prime time in:

@kv2019i
Copy link
Collaborator Author

kv2019i commented Oct 11, 2023

#8297 merged, so this feature request is now moot (build time dependency within one repository is ok).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request unclear Unclear enhancement request
Projects
None yet
Development

No branches or pull requests

7 participants