Skip to content

Conversation

david-cermak
Copy link
Collaborator

@david-cermak david-cermak commented Mar 7, 2023

Simplified the workflow, use idf-build-apps package to find apps for all supported targets.

Additional changes:

  • removed IDFv4.1 from tests
  • added common build_apps.py utility for finding/building apps
  • restructured workflow files and rules:
    • <component>__<test_type>.yml -- component related test workflow (need the component label to run it in PR)
    • common_check.yml -- runs unconditionally

Future changes:

  • all components to use local build_apps.py for all targets
  • simplify upload_artifact job using ci/clean_build_artifacts.sh (as done in esp_modem target tests)
  • build for all combination of supported IDF versions per component

@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch 17 times, most recently from 2cb6e99 to 3b537a3 Compare March 7, 2023 19:57
@david-cermak david-cermak marked this pull request as ready for review March 7, 2023 20:26
@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch from 3b537a3 to 0c9b51b Compare March 8, 2023 09:53
@david-cermak david-cermak changed the title fix(esp_modem): remove IDFv4.1 from tests fix(esp_modem): run CI build job for all targets Mar 8, 2023
Also
* removed IDFv4.1 from tests
* added common build_apps.py utility for finding/building apps
Some targets don't support UART_SCLK_APB (e.g. esp32c2)
This change is applicable only for IDFv5.0 and later, as older version
didn't define UART_SCLK_DEFAULT.

Closes espressif#241
@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch from 0c9b51b to e6beccb Compare March 8, 2023 14:52
@david-cermak david-cermak added modem and removed mdns labels Mar 8, 2023
@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch from e6beccb to cbda157 Compare March 8, 2023 16:34
@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch from 9a1c8ac to fd8eda8 Compare March 8, 2023 19:02
@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch from fd8eda8 to 1bea39a Compare March 8, 2023 19:15
@david-cermak david-cermak self-assigned this Mar 8, 2023
Copy link
Contributor

@gabsuren gabsuren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question regarding full test set running. Do we need such label also?

@gabsuren gabsuren self-requested a review March 15, 2023 07:31
@gabsuren
Copy link
Contributor

gabsuren commented Mar 15, 2023

LGTM!
2 questions

  1. Wouldn't it be nice also have a label run_all or smt. to be able run all test sets?
  2. When merging to the master all test sets should be run automatically, in this case will they be executed in that way?

@david-cermak
Copy link
Collaborator Author

Thanks for the review, both points make sense -- will address, thanks!

  1. Wouldn't it be nice also have a label run_all or smt. to be able run all test sets?

good point, will add support for this new label to run everything.

  1. When merging to the master all test sets should be run automatically, in this case will they be executed in that way?

ATM it would check the modification paths to run only relevant tests. But I agree that we should check all components after merge to keep master branch clean,. will fix,

@david-cermak david-cermak force-pushed the bugfix/modem_strip_idf_4.1 branch from 1bea39a to 13eca12 Compare March 15, 2023 14:38
@david-cermak david-cermak added modem and removed mdns labels Mar 15, 2023
@david-cermak
Copy link
Collaborator Author

  1. Wouldn't it be nice also have a label run_all or smt. to be able run all test sets?

I've decided not to implement one common label, since it would unnecessarily complicate the conditions (and would need to apply it everywhere). we can always add it in future if needed. for now, I don't think we'd have too many PRs affecting multiple components and if so, we can add more labels.

-    if: contains(github.event.pull_request.labels.*.name, 'mdns')
+    if: $${ contains(github.event.pull_request.labels.*.name, 'mdns') && contains(github.event.pull_request.labels.*.name, 'run_all') }}
  1. When merging to the master all test sets should be run automatically, in this case will they be executed in that way?

Done.

@david-cermak david-cermak merged commit ba45e9a into espressif:master Mar 15, 2023
david-cermak added a commit to david-cermak/esp-protocols that referenced this pull request Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants