Skip to content
This repository was archived by the owner on Apr 1, 2022. It is now read-only.

Add ability for libraries actions to report changes in memory usage #18

Merged
merged 6 commits into from
Mar 30, 2020
Merged

Add ability for libraries actions to report changes in memory usage #18

merged 6 commits into from
Mar 30, 2020

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Mar 11, 2020

Memory usage deltas reporting

Update to the compile-examples action

If the enable-size-deltas-report input is set to true, on every push to a pull request the change in memory usage of the example sketch specified via the compile-examples action's size-report-sketch input between the PR's head and base branch will be:

NOTE: Due to the current non-reporting of RAM usage, and misreporting of flash usage of Arduino SAMD Boards and Arduino SAM boards, I added a somewhat hacky workaround of using the compiler size tool to determine the memory usage when a board of one of those platforms is compiled for. This situation has already been resolved for Arduino SAMD Boards (arduino/ArduinoCore-samd#503), but there has not been a release since. For all other boards, arduino-cli's output is parsed for the memory usage data, which provides support for all boards without the need to add a lot of complexity to the action's script to reproduce arduino-cli's handling of tools. WORKAROUND CODE HAS NOW BEEN REMOVED

Addition of report-size-deltas action

The newly added report-size-deltas action converts the data from the workflow artifact saved during the compile-examples workflow into a Markdown formatted table that is commented to the PR's thread:
https://github.com/per1234/Arduino_ConnectionHandler/pull/1#issuecomment-605624787
Clipboard01
The "N/A" shown in the table above for adafruit:samd:adafruit_feather_m0's RAM is because, like Arduino SAMD Boards, Adafruit SAMD Boards does not display RAM usage in the output. However, I didn't provide the workaround I noted above for Adafruit boards. I added that board to the demonstration to show how the action handles this situation.

The "N/A" shown in the table above for the arduino:samd:mkrgsm1400's flash and RAM is because the example sketch didn't compile in the PR's base branch, but does compile in the PR's head branch. I set the demonstration up this way to show how the action handles this situation.

This action is intended to be used in a scheduled workflow. The reason for this is that the automatically generated GitHub access token is downgraded to read-only when the pull request is submitted from a fork, making it impossible to comment on the PR thread from an action triggered by a pull_request event. There are three downsides to the scheduled event approach:

  • The report is delayed by as much as the schedule interval (5 minutes minimum).
  • The repository owner will get one email for every workflow run if they have Settings > Notifications > GitHub Actions > Send notifications for failed workflows only unchecked. The default setting is unchecked. Those who do have that setting unchecked can either check it or use an email filter.
  • If scheduled on a short interval, the workflow will use up a significant number of minutes, likely in excess of the free minutes allowance GitHub provides for private repositories. Each workflow run will be around 30 seconds. At the minimum 5 minute interval this will use around 4320 minutes/month/repository. Public repositories get unlimited minutes, so this is only a concern for private repositories.

Memory usage trends reporting with the compile-examples action

If the enable-size-trends-report input is set to true, on every push to the default branch the memory usage of the example sketch is written to a Google Sheets spreadsheet (example). This can be used to track the changes in memory usage over time.

…ntroduced by a PR

Changes in the flash and dynamic memory for global variables usage of an example sketch are determined during the pull request build by the compile-examples action. The action displays the memory usage data in the log and also saves it to a JSON formatted file.

The actions/upload-artifact action may be used to create a workflow artifact containing the memory usage data files.

The newly added arduino/actions/libraries/report-size-deltas action may be used to provide a table summarizing the memory usage changes as a comment in the pull request thread.
@per1234
Copy link
Contributor Author

per1234 commented Mar 12, 2020

Regarding this:

If scheduled on a short interval, the workflow will use up a significant number of minutes, likely in excess of the free minutes allowance GitHub provides for private repositories

Before I discovered the issue with the access token permissions level downgrade, I wrote a version of the report-size-deltas action which was intended to be triggered by a pull_request event:
https://github.com/per1234/actions/tree/original-report-size-deltas/libraries/report-size-deltas
That approach only works for PRs submitted from a branch.

The most common development approach at Arduino seems to be to work from a branch of the target repository (rather than a fork), then submit a PR from that branch. With this approach, PRs to private repositories from forks will be rare. So the workaround for the problem of the scheduled report-size-deltas workflow using up too many minutes in private repositories would be to add some code to the report-size-deltas action so that:

  • If triggering event is schedule: work as the report-size-deltas action provided in this PR does.
  • If the triggering event is pull_request and the PR comes from a branch: work as the action I linked above does.
  • If the triggering event is pull_request and the PR comes from a fork: soft exit.

So when the action is used from a private repo, the user has the option of:

  • Support reports for PRs from forks, at the cost of using more minutes and/or delayed reports.
  • Don't use a significant number of minutes and provide reports quickly, at the cost of not supporting reports for PRs from forks.

This wouldn't require any change to the action's API. It is purely determined by the user's GitHub Actions workflow configuration.

@aentinger aentinger self-requested a review March 16, 2020 12:10
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.

Hi @per1234 👋 Fantastic work you did here. I'm very impressed ;) I've broached the subject of creating a new release of ArduinoCore-samd with the firmware team. This would save us on using the workaround you've developed for SAM/SAMD boards. I'd be willing to exclude the SAM boards from this feature as to my knowledge there will be no new SAM boards and there haven't been many old ones (anything except Arduino Due?). What do you think?

@per1234
Copy link
Contributor Author

per1234 commented Mar 16, 2020

Thanks! I appreciate your kind words.

This would save us on using the workaround you've developed for SAM/SAMD boards.

Sounds good to me. It would be nice to eliminate the unnecessary extra complexity added by that workaround code.

The action will still work for the flash reported in the arduino-cli output on the Due (though that doesn't include the data section). It will just report "N/A" for RAM.

@aentinger
Copy link
Contributor

Here you go ... ArduinoCore-samd 1.8.6 containing the required fixes has just been released. Please let me know when your rework is complete, I'll do another review then and we should be good to go 😉

@aentinger
Copy link
Contributor

Hi @per1234 👋 Just wanted to ask if you got around to clean-up this PR after the SAMD core release?

@per1234
Copy link
Contributor Author

per1234 commented Mar 23, 2020

@aentinger I haven't done it yet, but I will get on it.

@per1234
Copy link
Contributor Author

per1234 commented Mar 23, 2020

I have now removed the workaround code in abcb285

@per1234
Copy link
Contributor Author

per1234 commented Mar 23, 2020

@aentinger just a reminder that since this introduces the use of secrets, I would like to get a security review before the PR is merged. Before doing that, I'd appreciate if you'd take a look at abcb285 to see if you spot any problems.

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.

LGTM 👍 Thank you for the hard work. @gvarisco can you please take a look at it from the infosec perspective? @per1234 can fill you in in case you've got questions.

per1234 added 2 commits March 23, 2020 05:18
When the feature is enabled, memory usage data for a selected example sketch is published to a Google Sheets spreadsheet on every push to the repository's default branch.
… fully report sizes

The issue of RAM use by global variables not being reported and flash being incorrectly reported by the Arduino SAMD Boards platform has been fixed in the 1.8.6 release. The issue remains for the Arduino SAM Boards platform, but it's not worth the added complexity just to improve size reporting for the Due.
@gvarisco gvarisco requested a review from rsora March 23, 2020 12:21
@gvarisco
Copy link

Adding also @rsora as a reviewer as he spent quite some time on GitHub Actions.

@per1234
Copy link
Contributor Author

per1234 commented Mar 23, 2020

Hi @gvarisco, Here are the items of interest:
This PR introduces the use of two secrets into the compile-examples action:

github-token
This is used for authentication with the GitHub API.
I did not specify a default value for this input because it is only needed when people are using the size reporting features of the action.
Most people will be using ${{ secrets.GITHUB_TOKEN }}.
I pass this to entryfile.sh as a command line argument. The value is masked in the workflow build logs.
Question: would it be better if I passed this to entryfile.sh as an environment variable? I used the argument approach to be consistent with the previously established system for passing the inputs, but I already had to deviate from that convention for the Google key file.

keyfile
The contents of the Google key file containing the credentials for editing a Google Sheets spreadsheet.
I provided instructions for generating this file and creating the GitHub secret in the action's readme. I'd appreciate it if you would take a look at those instructions as well in case you spot any issues on setting things up on the Google end of things.
I pass the key file to entryfile.sh using the automatically generated environment variable INPUT_KEYFILE. The reason for this is I discovered that GitHub's secret masking feature only partially masked this, exposing the private_key field of the file in the logs!
I pass it to reportsizetrends.py as a command line argument.


The newly added report-size-deltas action uses one secret:
github-token
This is used for authentication with the GitHub API.
The default value is ${{ github.token }}.
It is passed to reportsizedeltas.py using the automatically generated environment variable INPUT_GITHUB-TOKEN.

Copy link

@gvarisco gvarisco left a comment

Choose a reason for hiding this comment

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

I've heard there have been further discussions. Leaving this to Luigi and Roberto for future review.

@gvarisco gvarisco requested review from gvarisco and removed request for gvarisco March 25, 2020 17:34
Copy link
Contributor

@rsora rsora left a comment

Choose a reason for hiding this comment

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

Excellent work @per1234 👍
Left various comment regarding how to improve dependency management.

Side notes for future submissions: Try to split as much as possible into smaller PRs representing self contained changes/additions, I understand that all this work is all related, but having smaller PRs makes easy to do reviews.

per1234 and others added 3 commits March 29, 2020 03:54
- Move the dependencies installations out of entrypoint.sh
- Pin Python version to 3.8.2
Pin Python version to 3.8.2

Co-Authored-By: Roberto Sora <r.sora@arduino.cc>
reportsizedeltas.py -> /reportsizedeltas.py
@rsora rsora merged commit 8845005 into arduino:master Mar 30, 2020
@per1234 per1234 deleted the size-reports branch March 30, 2020 07:46
@aentinger
Copy link
Contributor

Great work 👍 Looking forward to integrate with firmware repos ;)

@per1234 per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Sep 29, 2021
@per1234 per1234 self-assigned this Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants