Repository navigation
xtensa-build-zephyr.py: fix find_west_workspace() to return west topdir - #5906
Conversation
When neither -c nor -p is passed, the script defaults to '-p SOF_TOP/zephyrproject', initializing the `west_top` global with that. When everything is already cloned, this becomes zephyrproject/modules/audio/sof/zephyrproject which is really not a `west topdir`. Then west is pointed at: real_west_top/modules/audio/sof/zephyrproject/zephyr/samples/subsys/audio/sof which obviously fails. This was found while testing https://thesofproject.github.io/latest/getting_started/build-guide/build-with-zephyr.html and trying (and failing) to switch away from the older .sh script. Fix find_west_workspace() to return the output of `west topdir` and use that to update the `west_top` global. This bug was introduced from the very start when the script was converted from shell to python. The shell script did not have this problem because it could be used without using neither -c nor -p. This logical difference was mentioned in the code review: thesofproject#5299 (comment) Also remove a couple of accidental creations of a sof/zephyrproject/ directory when sof is already inside a higher zephyrproject. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
04b5100 to
aecce52
Compare
|
https://sof-ci.01.org/sofpr/PR5906/build608/devicetest/ has the unfortunately usual suspend/resume and disk failures, nothing related to this. |
Add missing `git submodule update` as reported by @plbossart (thx!) Also make the documentation ready for `xtensa-build-zephyr.py` which... does not have any hack to automatically download git submodules like the older .sh script has. As a side-effect, this breaks down the process and adds an intermediate `west build` step which removes one layer of indirection in case of some Zephyr environment issue. Cannot fully switch to the Python version yet because of bugs like thesofproject/sof#5906 but do recommend users to start trying it. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Add missing `git submodule update` as reported by @plbossart (thx!) Also make the documentation ready for `xtensa-build-zephyr.py` which... does not have any hack to automatically download git submodules like the older .sh script has. As a side-effect, this breaks down the process and adds an intermediate `west build` step which removes one layer of indirection in case of some Zephyr environment issue. Cannot fully switch to the Python version yet because of bugs like thesofproject/sof#5906 but do recommend users to start trying it. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
| sof_symlink = pathlib.Path(audio_modules_dir, "sof") | ||
| # Symlinks creation requires administrative privileges in Windows or special user rights | ||
| try: | ||
| if not sof_symlink.is_symlink(): |
There was a problem hiding this comment.
Please do not remove this check.
It is understandable to check for symlink target existance however we would like to also be sure, that it is symlink not the actual directory or file (it would break the sense of all script).
I think the complete solution would be:
- Check if path exists
- If exists check if it's a symlink
- If exists but not symlink - ... issue error? Just deleting directory with sof may be dangerous?
- else create new symlink
There was a problem hiding this comment.
I'm afraid this is an important part of the fix. I'm not sure how you've been using this script but when simply following the most basic west init; west update instructions at https://thesofproject.github.io/latest/getting_started/build-guide/build-with-zephyr.html then west_top/modules/audio/sof exists and is not a symlink.
Again the problem is that this script defaults to -p, the older shell script defaulted to neither to support the most basic west init; west update situation.
There was a problem hiding this comment.
Marc, those docs are outdated. I've even started fixing them but can't find time now due to upstream process.
As far as I remember I agreed on design that zephyr is inside of sof not otherwise and we resolve it using this symlink so you dont' have zephyr inside sof with own sof (and probably another zephyr?).
Theoretically if you take manual approach using west commands you can go the other way - download zephyrproject and then use SOF that is inside modules/audio/sof... But is it worth it?
SOF is Zephyr application, script xtensa-build-zephyr is in the SOF repository, SOF is a build starting point. It generally makes more sense (in my opinion) that SOF contains Zephyr not the otherwise.
Again the problem is that this script defaults to -p
As I previously said, its by design.
I will not oppose against what you've said, if you wish this script to work both ways then I will just drop and go on with existing changes. Up to you Mark.
There was a problem hiding this comment.
I think you misunderstand me: I want to support ALL possible directory setups! Mine, yours and anyone else's. So I don't think these instructions are "outdated" (and I linked to a new sof-docs PR in the description. EDIT: it was merged).
The script should IMHO especially support the most basic west init; west update setup = the one that does not require any symlink or any other magic.
The older shell script supported all possible directory setups so there's no reason why the python script cannot and I think this fix achieves that!
There was a problem hiding this comment.
If it works both ways lets keep it.
There was a problem hiding this comment.
@aborisovich @marc-hb Let's keep both ways. One will be used by Zephyr developers for whom SOF is just another module (focus on Zephyr development), another used by SOF developers (focus on SOF development with a fixed Zephyr commit used to build the SOF application). We may still unify to a single model, but I don't think we should arbitrarily limit the usage especially if we have existing documentation out there.
Add missing `git submodule update` as reported by @plbossart (thx!) Also make the documentation ready for `xtensa-build-zephyr.py` which... does not have any hack to automatically download git submodules like the older .sh script has. As a side-effect, this breaks down the process and adds an intermediate `west build` step which removes one layer of indirection in case of some Zephyr environment issue. Cannot fully switch to the Python version yet because of bugs like thesofproject/sof#5906 but do recommend users to start trying it. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
5 participants
When neither -c nor -p is passed, the script defaults to '-p
SOF_TOP/zephyrproject', initializing the
west_topglobal withthat. When everything is already cloned, this becomes
zephyrproject/modules/audio/sof/zephyrproject which is really not a
west topdir. Then west is pointed at:real_west_top/modules/audio/sof/zephyrproject/zephyr/samples/subsys/audio/sof
which obviously fails.
This was found while testing
https://thesofproject.github.io/latest/getting_started/build-guide/build-with-zephyr.html
and trying (and failing) to switch away from the older .sh script.
Fix find_west_workspace() to return the output of
west topdirand usethat to update the
west_topglobal.This bug was introduced from the very start when the script was
converted from shell to python. The shell script did not have this
problem because it could be used without using neither -c nor -p. This
logical difference was mentioned in the code review:
#5299 (comment)
Also remove a couple of accidental creations of a sof/zephyrproject/
directory when sof is already inside a higher zephyrproject.
Signed-off-by: Marc Herbert marc.herbert@intel.com