[SKIP CI] .github/zephyr.yml: add a temporary submodules/west consistency check #7320
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
marc-hb
changed the title
Test new, fancy and impressive looking git --filter optimization on non-critical "manifest check". Extend it later to other checks if it hasn't caused any issue. https://github.blog/2020-12-21-get-up-to-speed-with-partial-clone-and-shallow-clone/ zephyrproject-rtos/west#638 (comment) Signed-off-by: Marc Herbert <[email protected]>
marc-hb
force-pushed
the
submodules-consistency
branch
from
bc17647 to
76f82af
Compare
|
|
|
The "git submodules" consistency check added by the 2nd commit failed in https://github.com/thesofproject/sof/actions/runs/4486669431/jobs/7889366353?pr=7320 which is exactly its purpose. This was just broken by PR #7250. EDIT: and fixed by #7316 checkpatch fails because it wants me to split an URL. I decline. |
marc-hb
requested review from
dbaluta,
cujomalainey,
lgirdwood and
aborisovich
as code owners
marc-hb
requested review from
softwarecki,
kv2019i,
aiChaoSONG,
andyross,
greg-intel,
juimonen and
keqiaozhang
abonislawski
approved these changes
Mar 22, 2023
softwarecki
approved these changes
Mar 22, 2023
lgirdwood
reviewed
Mar 22, 2023
| if git status --porcelain=v2 | grep ^ ; then | ||
| git status | ||
| echo 'FAIL: inconsistency between git submodules and west.yml!' | ||
| echo 'Always use "git status"' |
There was a problem hiding this comment.
What are you telling people to do here ? By itself git status wont fix the problem.
Btw, why not just convert rimage to west and avoid temp W/A ?
There was a problem hiding this comment.
What are you telling people to do here ? By itself git status wont fix the problem.
The error message says "inconsistency between git submodules and west.yml" and the check name is "git submodule consistency". There's also a comment in west.yml above the line that people changed.
Btw, why not just convert rimage to west and avoid temp W/A ?
That's explained in the comment above. rimage is already in west, the longer-term fix is to change its location so it does coincide with the XTOS submodule. However this will (temporarily) break CI as every rimage change does as we just saw in zephyrproject-rtos/zephyr#54700 and zephyrproject-rtos/zephyr#56099
There was a problem hiding this comment.
@lgirdwood I added a direct reference to the detailed rimage submodule instructions in sof/west.yml, I should have done that from the start.
Real-world experience has proved again that big READMEs are not enough, not even when they're only a couple lines away as the one added in commit 8fd351e ("west.yml: add warning to keep git submodules in sync"). Only some failure / red color stand a chance. It also seems some people rarely use "git status". This was discovered in commit d9eb16a ("cmake: add warning when git submodule changes are found") but is still surprising. Signed-off-by: Marc Herbert <[email protected]>
marc-hb
force-pushed
the
submodules-consistency
branch
from
76f82af to
7d82f64
Compare
marc-hb
commented
Mar 22, 2023
There was a problem hiding this comment.
The rimage submodule has just been updated elsewhere, this new test is now passing in https://github.com/thesofproject/sof/actions/runs/4492799010/jobs/7903159878?pr=7320
| if git status --porcelain=v2 | grep ^ ; then | ||
| git status | ||
| echo 'FAIL: inconsistency between git submodules and west.yml!' | ||
| echo 'Always use "git status"' |
There was a problem hiding this comment.
@lgirdwood I added a direct reference to the detailed rimage submodule instructions in sof/west.yml, I should have done that from the start.
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]>
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.
Real-world experience has proved again that big READMEs are not enough, not even when they're only a couple lines away as the one added in commit 8fd351e ("west.yml: add warning to keep git submodules in sync"). Only some failure / red color stand a chance.
It also seems some people rarely use "git status". This was discovered in commit d9eb16a ("cmake: add warning when git submodule changes are found") but is still surprising.