Add function to handle library installation from ci.json #11766
Conversation
JakubAndrysek
requested review from
pedrominatel and
lucasssvaz
as code owners
👋 Hello JakubAndrysek, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 76 files 76 suites 13m 9s ⏱️ Results for commit eac284b. ♻️ This comment has been updated with latest results. |
P-R-O-C-H-Y
added
Type: CI & Testing
There was a problem hiding this comment.
Pull Request Overview
This pull request adds functionality to automatically install Arduino libraries from a ci.json configuration file during the sketch build process. It enhances the build automation by ensuring required libraries are available before compilation begins.
- Added a new
install_libsfunction that parsesci.jsonfiles and installs specified libraries - Integrated library installation into the build process to run before sketch compilation
- Updated documentation to describe the new
libsfield inci.jsonfiles
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/scripts/sketch_utils.sh |
Implements the install_libs function and integrates it into the build process |
docs/en/contributing.rst |
Documents the new libs field for specifying required libraries in ci.json |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.github/scripts/sketch_utils.sh
Outdated
| for lib in $libs; do | ||
| if [[ "$lib" == https://github.com/* ]]; then | ||
| needs_unsafe=true | ||
| break | ||
| fi | ||
| done |
There was a problem hiding this comment.
The pattern matching https://github.com/* is too restrictive and may miss other valid Git URLs. Consider using a more comprehensive pattern like https://* or http*://*.git* to handle various Git hosting services and URL formats.
Copilot uses AI. Check for mistakes.
.github/scripts/sketch_utils.sh
Outdated
| for lib in $libs; do | ||
| [ "$verbose" = true ] && echo "Processing library: $lib" | ||
|
|
||
| if [[ "$lib" == https://github.com/* ]]; then |
There was a problem hiding this comment.
This pattern matching is duplicated from line 663. Consider extracting this logic into a helper function or variable to avoid code duplication and ensure consistency.
| if [[ "$lib" == https://github.com/* ]]; then | |
| if is_github_url "$lib"; then |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
.github/scripts/sketch_utils.sh
Outdated
| else | ||
| output=$("$ide_path/arduino-cli" lib install --git-url "$lib" 2>&1) | ||
| install_status=$? | ||
| [ $install_status -ne 0 ] && echo "$output" | grep -Ei "error|warning|warn" >&2 || true |
There was a problem hiding this comment.
The error filtering logic is duplicated. Consider extracting this into a function or variable to reduce code duplication and improve maintainability.
| [ $install_status -ne 0 ] && echo "$output" | grep -Ei "error|warning|warn" >&2 || true | |
| filter_and_report_errors "$install_status" "$output" |
Copilot uses AI. Check for mistakes.
.github/scripts/sketch_utils.sh
Outdated
| else | ||
| output=$("$ide_path/arduino-cli" lib install "$lib" 2>&1) | ||
| install_status=$? | ||
| [ $install_status -ne 0 ] && echo "$output" | grep -Ei "error|warning|warn" >&2 || true |
There was a problem hiding this comment.
The error filtering logic is duplicated. Consider extracting this into a function or variable to reduce code duplication and improve maintainability.
| [ $install_status -ne 0 ] && echo "$output" | grep -Ei "error|warning|warn" >&2 || true | |
| print_error_warnings $install_status "$output" |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Hi @JakubAndrysek, thanks for your contribution.
The docs part looks good to me.
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Do you have any plans for this functionality or is it required for something ? I worry that using libraries in tests will constantly cause issues as we make breaking changes from time to time and will need to wait until the lib author fixes it. It wouldn't be nice to have a broken test for a long time just because some library is abandoned. If it's for testing the libraries itself we have the external libraries test that can be expanded to add any lib needed. |
|
Hi, I am preparing new test scenarios; some will run only in Wokwi. Back to the question of external libraries, the aim is to depend on "our" libraries, which are maintained alongside the core libraries, but do not necessarily have to be part of the core repository. |
|
Understood. In this case it would be better to create it under the espressif organization. But AFAIK none of us devs have permission to do that. We can discuss it on the meeting. |
Description of Change
This pull request enhances the automation for building Arduino sketches by introducing support for installing required libraries specified in a
ci.jsonfile. It adds a newinstall_libsfunction to the build scripts, ensures libraries are installed before building sketches, and updates the documentation to describe this new feature.Test Scenarios
I have tested multiple scenarios of library installations from
ci.jsonfiles on various ESP32 boards in CI.Link to my GitHub Actions workflow: link
Related links