Revert "xtensa: remove ELF section address rewriting" #56099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zephyrbot
added
platform: Intel ADSP
zephyrbot
requested review from
aborisovich,
ceolin,
galak,
gopiotr,
hakehuang,
kv2019i,
lgirdwood,
lyakh,
marc-hb,
MaureenHelm,
PerMac,
softwarecki and
tmleman
carlescufi
previously approved these changes
Mar 22, 2023
kv2019i
previously approved these changes
Mar 22, 2023
There was a problem hiding this comment.
Ack. FYI to @lyakh
I think the accurate dependency is that this requires a newer rimage (thesofproject/rimage@63eb6ff or newer) to build flashable images (for platforms requiring rimage-created binaries).
yes, we will look into adding rimage dependency in zephyr CI. The blocker is that we have to build rimage which is not straightforward in upstream CI right now. |
tmleman
previously approved these changes
Mar 22, 2023
|
@nashif I think you added by accident the revert on top of modification of retry error functionality in twister. Shouldn't the revert be an independent commit? |
This reverts commit 7a85983. This commit was merged prematurely and is causing issues on multiple platforms. Signed-off-by: Anas Nashif <[email protected]>
nashif
dismissed stale reviews from tmleman, kv2019i, and carlescufi
via
a4c0874
nashif
force-pushed
the
revert_elf_rewrite
branch
from
d317532 to
a4c0874
Compare
PerMac
approved these changes
Mar 22, 2023
softwarecki
approved these changes
Mar 22, 2023
|
For the record and backlink, similar rimage dependency issues blocked my #54700 - except I now found and tested backwards compatibility solutions in the last version. |
marc-hb
added a commit
to marc-hb/rimage
that referenced
this pull request
Apr 18, 2023
The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - thesofproject#155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>
marc-hb
added a commit
to marc-hb/rimage
that referenced
this pull request
Apr 18, 2023
The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - thesofproject#155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>
kv2019i
pushed a commit
to thesofproject/rimage
that referenced
this pull request
Apr 26, 2023
The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - #155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>
|
Looking at this since it's going to be re-submitted eventually. When you do so please don't leave comments like this in place: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 7a85983 / #55554
This commit was merged prematurely and is causing issues on multiple
platforms.
Signed-off-by: Anas Nashif [email protected]