-
Notifications
You must be signed in to change notification settings - Fork 1.5k
arch/xtensa/esp32[-s2|-s3]: Modify the method of downloading the repository #17236
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
Conversation
05d05e7 to
eac577d
Compare
ed60e98 to
067398b
Compare
|
@TangMeng12 Please look here The idea behind this logic was to reduce the time needed for the jobs. With the change you want to make, build times on CI will be longer than before. |
Throughout NuttX, and mostly in the I think we need to discuss (on the mailing list) and come up with a systematic approach that balances everyone's needs in a suitable way. Some of those needs are:
Rather than maintaining a hodgepodge of different approaches, a systematic approach will make it easier to add new external dependencies, maintain existing ones, and handle licensing correctly. |
@simbit18, can you add a line to the documentation on how to use the idea proposed in #13301? |
I have updated the commit based on your suggestions. In the commit, the default value of |
73db8b4 to
826faa4
Compare
|
@TangMeng12 , allow me to make a few observations. As already mentioned #13301, the current system was designed to speed up and reduce errors during the cloning/download step (also possible with Curl downloads if desired) on our CI. This system on CI makes sense when the packages need to be used in multiple configurations (currently only for Arch Expressif ) jobs Linux (risc-v-04), Linux (xtensa-01), Linux (xtensa-02) With this simple check
We can insert any package (if necessary) from the Nuttx or Apps repository into the storage directory. Storage is enabled with -S adds the nxtmpdir folder for third-party packages. in the build.yml workflow on GitHub nuttx/.github/workflows/build.yml Line 194 in 201406b
and if desired, you can use it locally
Line 35 in 201406b
At the moment, simply using CONFIG_ALLOW_DOWNLOADS (must be added to all Expressif configurations!) does not meet the requirements of our CI! @TangMeng12 Converted back to draft to avoid accidental merging. The impact must be considered !!! |
Directly downloading the Git repository is inconvenient for local debugging. This will allow to automatically download external packages from the Internet. If not set, the repo need to be download will need to provide them manually, otherwise an error will occur and the build will be aborted. Add `USE_NXTMPDIR_ESP_REPO_DIRECTLY`, with this we can use `USE_NXTMPDIR_ESP_REPO_DIRECTLY=y make` which can directly use esp-hal-3rdparty under nxtmpdir without CLONE, CHECK_COMMITSHA, reset, checkout and update. Just `cp -rf nxtmpdir/esp-hal-3rdparty chip/$(ESP_HAL_3RDPARTY_REPO)`. Signed-off-by: v-tangmeng <v-tangmeng@xiaomi.com>
tmedicci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TangMeng12 . I agree with this solution.
@simbit18 @fdcavalcanti and @eren-terzioglu , can you PTAL?
|
@TangMeng12 Could you help me with how to use this feature? I would like to avoid having to download the repository each time after a |
|
Hi @Laczen step used for our CI only for esp32
-S adds the nxtmpdir folder for third-party packages. |
Hi @simbit18, thanks for the reply. I am doing the following: So I think that it is needed to perform an extra step. |
Set the environment variable NXTMPDIR to "../nxtmpdir" and try again. |
|
@Laczen test performed now
The first time, it creates the nxtmpdir folder and clones esp-hal-3rdparty (https://github.com/espressif/esp-hal-3rdparty.git).
The next time, it verifies that the correct esp-hal-3rdparty exists and does not clone it but copies the folder. |
@simbit18 Thanks for the reply. Could you tell me what to expect under /nxtmpdir/esp-hal-3rdparty. I am expecting to see the complete hal with components there so that they are not removed when Could you verify that your above local test does not redownload after a I am also expecting to see a link to nxtmpdir/esp-hal-3rdparty under ../nuttx/arch/xtensa/src/chip/esp-hal-3rdparty, but |
|
@Laczen as mentioned earlier, option -S make distclean removes esp-hal-3rdparty (only the copied one, of course :) -> chip/$(ESP_HAL_3RDPARTY_REPO) ) with all subsequent steps after cloning. so only the clone phase is skipped |
|
@simbit18 probably my expectations are wrong. What I see under /nxtmpdir/esp-hal-3rdparty is the main branch of espressif/esp-hal-3rdparty. The main branch only contains a tools directory (with one script file) and a LICENSE and README.md. I would expect to see the release/master.a (or similar) branch instead. At the moment it is not clear for me why the clone of the main branch is performed. |
|
@Laczen This is the system adopted by Espressif. However, the folder is a total of 300 MB. |
|
@simbit18 please don't take my question as criticism on the chosen solution. Not only the espressif CI could benefit from the chosen solution. If a small change/better user understanding would allow avoiding downloads after |
|
@simbit18 I have been able to get it working by only checking out the release/master.a branch to nxtmpdir, initializing the submodules in the nxtmpdir, applying the patches in nxtmpdir and change the Make.def to only applying the patches if IMHO it is also possible to replace the copying of files with a link (at least on linux). |
|
@simbit18, with one very small change in Config.mk line 638: |
Step 1: Download and initialize your local The reasons are as follows:
|
Directly downloading the Git repository is inconvenient for local debugging.
Directly downloading the Git repository is inconvenient for local debugging.
This will allow to automatically download external packages from the Internet.
If not set, the repo need to be download will need to provide them manually,
otherwise an error will occur and the build will be aborted.
Add
USE_NXTMPDIR_ESP_REPO_DIRECTLY, with this we can useUSE_NXTMPDIR_ESP_REPO_DIRECTLY=y makewhich can directly use esp-hal-3rdpartyunder nxtmpdir without CHECK_COMMITSHA, reset, checkout and update.
Note: Please adhere to Contributing Guidelines.
Summary
Add
USE_NXTMPDIR_ESP_REPO_DIRECTLY, with this we can useUSE_NXTMPDIR_ESP_REPO_DIRECTLY=y makewhich can directly use esp-hal-3rdpartyunder nxtmpdir without CHECK_COMMITSHA, reset, checkout and update.
nuttxworkspace
|
|- nuttx
|- apps
|- nxtmpdir
USE_NXTMPDIR_ESP_REPO_DIRECTLY=y make
ARCH
arch/xtensa/src/esp32/Make.defs
arch/xtensa/src/esp32s2/Make.defs
arch/xtensa/src/esp32s3/Make.defs
arch/risc-v/src/common/espressif/Make.defs
arch/risc-v/src/esp32c3-legacy/Make.defs
ARCH
arch/xtensa/src/esp32/Make.defs
arch/xtensa/src/esp32s2/Make.defs
arch/xtensa/src/esp32s3/Make.defs
arch/risc-v/src/common/espressif/Make.defs
arch/risc-v/src/esp32c3-legacy/Make.defs
Impact
There should be no impact.
Testing
CI