Skip to content
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

Merged
merged 28 commits into from
Nov 25, 2020

Conversation

giulcioffi
Copy link
Contributor

@giulcioffi giulcioffi commented Oct 15, 2020

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:

  • internal core libraries + examples
  • some of the most used libraries + related examples
  • all the basic examples provided in the Arduino IDE
  • Memory usage information collection is always enabled and handled by the report-size-deltas workflow.

.github/workflows/compile-examples.yml Outdated Show resolved Hide resolved
.github/workflows/report-size-deltas.yml Outdated Show resolved Hide resolved
Comment on lines 264 to 265
name: size-deltas-reports
path: size-deltas-reports
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor

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>
# MKRGSM1400 board
- board:
fqbn: "arduino:samd:mkrgsm1400"
additional-sketch-paths: |
Copy link
Contributor

@per1234 per1234 Nov 23, 2020

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"'
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# 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

Comment on lines 264 to 265
name: size-deltas-reports
path: size-deltas-reports
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Great work @giulcioffi !

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

Fantastic work @giulcioffi 🚀

@aentinger aentinger merged commit c7369ba into arduino:master Nov 25, 2020
@giulcioffi giulcioffi deleted the CIworkflowWithAPI branch November 26, 2020 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants