-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
Add CI workflows (to be used with ArduinoCore-API integration) #567
Conversation
name: size-deltas-reports | ||
path: size-deltas-reports |
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.
name: size-deltas-reports | |
path: size-deltas-reports | |
name: sketches-reports | |
path: sketches-reports |
The default sketches report path of the arduino/compile-sketches
action and the default workflow artifact name of the arduino/report-size-deltas
action were changed by arduino/compile-sketches#7, so this change must accompany the actions name changes.
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.
Oops, I see I forgot about this and made a duplicate suggestion for the same change in my newer review below:
#567 (comment)
…tions Co-authored-by: per1234 <accounts@perglass.com>
ca58843
to
01f3e74
Compare
# MKRGSM1400 board | ||
- board: | ||
fqbn: "arduino:samd:mkrgsm1400" | ||
additional-sketch-paths: | |
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.
Since you set matrix.board.type
to usb
for this board, I suspect you intended for the paths under matrix.include[].board.type.usb
to be compiled for it, but what is happening is the value of the matrix.board.additional-sketch-paths
key is being overwritten. So it only ends up containing the single path ~/Arduino/libraries/MKRGSM/examples
, as you can see from this workflow log;
https://github.com/giulcioffi/ArduinoCore-samd/runs/1263161053?#step:7:16602
You can fix this by using a dedicated key for each, like matrix.board.additional-usb-sketch-paths
and matrix.board.additional-sketch-paths
. You can see an example of that here:
https://github.com/arduino/arduino-examples/blob/main/.github/workflows/compile-examples.yml
- ~/Arduino/libraries/MKRNB/examples | ||
# MKRWAN board | ||
- board: | ||
fqbn: '"arduino:samd:mkrwan1300" "arduino:samd:mkrwan1310"' |
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.
Since none of the existing matrix items match the matrix.include
filter, this creates a new matrix element.
https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#example-including-new-combinations
You can see the fqbn: '"arduino:samd:mkrwan1300" "arduino:samd:mkrwan1310"'
job here;
https://github.com/giulcioffi/ArduinoCore-samd/runs/1263161214
and the jobs for the fqbn: arduino:samd:mkrwan1300
and fqbn: arduino:samd:mkrwan1310
matrix elements:
https://github.com/giulcioffi/ArduinoCore-samd/runs/1263161020
https://github.com/giulcioffi/ArduinoCore-samd/runs/1263161034
Apparently the arduino/compile-sketches
action accepts the input fqbn: "arduino:samd:mkrwan1300" "arduino:samd:mkrwan1310"
and ends up just compiling only for the MKR WAN 1300.
You could add a general "WAN" attribute property to the matrix, something like what I did in this workflow:
https://github.com/arduino/arduino-examples/blob/main/.github/workflows/compile-examples.yml
The only problem is that, as you see here, when no matrix element matches the filter, one is created, which is why I had to define those properties for all the boards in the matrix, rather than only for the boards the property applied to.
You could also just add a separate matrix.include
filter for each of the MKR WAN boards board.fqbn
values.
This also has the same issue as I described above for arduino:samd:mkrgsm1400
of overwriting the previous value of additional-sketch-paths
for arduino:samd:mkrwan1300
.
- board: | ||
fqbn: "arduino:samd:mkrnb1500" | ||
additional-sketch-paths: | | ||
- ~/Arduino/libraries/MKRNB/examples |
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.
Same issue as I described above for arduino:samd:mkrgsm1400
of overwriting the previous value of additional-sketch-paths
.
- name: MFRC522 | ||
- name: Arduino_MKRMEM | ||
- name: FlashStorage | ||
- source-url: https://github.com/arduino-libraries/Keyboard.git |
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.
I'm guess that the use of the development version of the Keyboard library is because that version has an example sketch added:
arduino-libraries/Keyboard@a587425
but that example sketch is just a copy/paste of the KeyboardSerial example in the built-in examples:
https://github.com/arduino/arduino-examples/blob/main/examples/09.USB/Keyboard/KeyboardSerial/KeyboardSerial.ino
which the workflow is already configured to compile.
So there would be no loss of coverage from using the release version of the library and removing the ~/Arduino/libraries/Keyboard/examples/Serial
path from env.UNIVERSAL_SKETCH_PATHS
- name: MKRWAN | ||
- name: WiFiNINA | ||
platforms: | | ||
# Use Board Manager to install the latest release of Arduino megaAVR Boards to get the toolchain |
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.
# Use Board Manager to install the latest release of Arduino megaAVR Boards to get the toolchain | |
# Use Board Manager to install the latest release of Arduino SAMD Boards to get the toolchain |
name: size-deltas-reports | ||
path: size-deltas-reports |
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.
name: size-deltas-reports | |
path: size-deltas-reports | |
name: sketches-reports | |
path: sketches-reports |
This change is required due to a recent breaking change to the arduino/compile-sketches
action's default sketches report path and to the arduino/report-size-deltas
action's default workflow artifact name:
arduino/compile-sketches#7
arduino/report-size-deltas#7
…RWAN1300 and MKRWAN1310
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.
Great work @giulcioffi !
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.
Fantastic work @giulcioffi 🚀
In order to test that the ArduinoCore-API integration (#560 ) doesn't break the compilation of the main libraries/examples, a workflow for CI has been put in place. The
compile-examples
workflow compiles:report-size-deltas
workflow.