ci(workflow): Add esp-idf network examples build #124
Conversation
roma-jam
force-pushed
the
ci/add_esp_idf_network_examples
branch
from
084d360 to
2817f4a
Compare
roma-jam
changed the base branch from
master
to
fix/ncm_buffer_count_default_value
roma-jam
force-pushed
the
ci/add_esp_idf_network_examples
branch
from
2817f4a to
56cfcbb
Compare
roma-jam
force-pushed
the
ci/add_esp_idf_network_examples
branch
from
17462de to
3c4828c
Compare
There was a problem hiding this comment.
@roma-jam I can see that the CI is doing what you need, but it seems that it can be optimized a little
| @@ -47,6 +48,11 @@ def override_with_local_component_all(component, local_path, apps): | |||
|
|
|||
| # Go through all collected apps | |||
| for app in apps_with_glob: | |||
| # Verify that the app is a valid directory | |||
| if not path.isdir(app): | |||
There was a problem hiding this comment.
We use modern pathlib.Path instead of os.path in this file. Lets keep it that way for consistency
| if not path.isdir(app): | |
| if not Path(app).is_dir(): |
| @@ -11,18 +11,38 @@ jobs: | |||
| strategy: | |||
| matrix: | |||
| idf_ver: ["release-v5.0", "release-v5.1", "release-v5.2", "release-v5.3", "release-v5.4", "latest"] | |||
| target: ["esp32s2", "esp32s3", "esp32p4"] | |||
There was a problem hiding this comment.
Previously, we build all targets in one job. Now we have one job for each target, which is slower.
Could you please check if idf-build-apps --target all can handle the idf version correctly? Eg. it will not build for esp32p4 on idf release which do not support P4?
Please test this locally, so we do not onverload the test runners...
There was a problem hiding this comment.
Just confirmed this works. idf-build-apps --target all on idf v5.1.2 will not list ESP32-P4 target
FYI like this
[(v5.1.2)]> idf-build-apps find -p examples/peripherals/usb/host --recursive --manifest-file examples/peripherals/.build-test-rules.yml
| example: | ||
| - { name: "USB Device", path: "examples/peripherals/usb/device", manifest_path: "examples/peripherals"} | ||
| - { name: "Network", path: "examples/network", manifest_path: "examples/network" } |
There was a problem hiding this comment.
This creates another diminsion of the matrix, now we have 23 jobs for simple example builds, this should be optimized
number of jobs: idf_ver * target * example
| idf-build-apps find --path ${{ matrix.example.path }} --recursive --target ${{ matrix.target }} --manifest-file ${{ matrix.example.manifest_path }}/.build-test-rules.yml --build-dir build_@t_@w --work-dir @f_@t_@w | ||
| idf-build-apps build --path ${{ matrix.example.path }} --recursive --target ${{ matrix.target }} --manifest-file ${{ matrix.example.manifest_path }}/.build-test-rules.yml --build-dir build_@t_@w --work-dir @f_@t_@w |
There was a problem hiding this comment.
--manifest-files accepts multiple parameters, so you can pass both .build-test-rules.yml to one command
|
@tore-espressif Thanks for the review, will address all the notes asap. |
|
@tore-espressif I think optimization could take some time, so I think that this feature (have a example/network in CI) is not that important for the current fix-release. I think we might remove this PR from the release, to push it asap to get back on track. For fix-release, PTAL: #126 |
|
OK, agreed |
Description
This is a 3/3 PR to verify fixing the problem.
PR plan:
Related
Testing
N/A
Checklist
Before submitting a Pull Request, please ensure the following: