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 github action to report memory diff user diffsyms tool #6962

Merged
merged 1 commit into from
Jun 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/examples-esp32.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ jobs:
volumes:
- "/tmp/bloat_reports:/tmp/bloat_reports"
- "/tmp/output_binaries:/tmp/output_binaries"
- "/tmp/base_binaries:/tmp/base_binaries"

steps:
- name: Checkout
Expand Down Expand Up @@ -87,3 +88,24 @@ jobs:
# - name: Perform CodeQL Analysis
# if: ${{ github.event_name == 'push' }}
# uses: github/codeql-action/analyze@v1
- name: Download Base Binaries
uses: dawidd6/action-download-artifact@v2
if: ${{ github.event_name == 'pull_request' }}
with:
workflow: examples-esp32.yaml
name: esp32-example-build-${{ github.event.pull_request.base.sha }}
commit: ${{ github.event.pull_request.base.sha }}
path: /tmp/base_binaries
repo: project-chip/connectedhomeip
- name: Report symbol diff
if: ${{ github.event_name == 'pull_request' }}
run: |
pip3 install cxxfilt pyelftools humanfriendly pandas && \
Copy link
Contributor

Choose a reason for hiding this comment

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

should or could we install these packages in the docker image so that we do not run pip install on every commit?

We are trying to make CI faster. If possible, please create a separate PR that updates dockerfile and version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated scripts/requirements.in for this yesterday (#6975) so they should get in to future rolls.

./scripts/tools/memory/diffsyms.py --report-demangle \
/tmp/base_binaries/chip-all-clusters-app.elf \
/tmp/output_binaries/esp32-build/chip-all-clusters-app.elf \
| scripts/helpers/post_comment.py \
--github-api-token "${{ secrets.GITHUB_TOKEN }}" \
--github-repository project-chip/connectedhomeip \
--pr "${{ github.event.pull_request.number }}" \
--title "ESP32 Symbols Diff"
101 changes: 101 additions & 0 deletions scripts/helpers/post_comment.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
#!/usr/bin/env python3

#
# Copyright (c) 2021 Project CHIP Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

import argparse
import coloredlogs
import github
import logging
import traceback
import sys

def main():
"""Main task if executed standalone."""
parser = argparse.ArgumentParser(description='Fetch master build artifacts.')
parser.add_argument(
'--github-api-token',
type=str,
help='Github API token to upload the report as a comment')
parser.add_argument(
'--github-repository', type=str, help='Repository to use for PR comments')
parser.add_argument(
'--pr',
type=int,
help='The PR number of the pull request')
parser.add_argument(
'--title',
type=str,
help='Title of the comment')
parser.add_argument(
'--log-level',
default=logging.INFO,
type=lambda x: getattr(logging, x),
help='Configure the logging level.')
args = parser.parse_args()

# Ensures somewhat pretty logging of what is going on
logging.basicConfig(
level=args.log_level,
format='%(asctime)s - %(name)s - %(levelname)s - %(message)s')
coloredlogs.install()

if not args.github_api_token:
logging.error('Required arguments missing: github api token is required.')
return

try:
logging.info('Uploading report to "%s", PR %d', args.github_repository, args.pr)
logging.info('Title "%s"', args.title)

rawText = sys.stdin.read()
logging.info('Content "%s"', rawText)

api = github.Github(args.github_api_token)
repo = api.get_repo(args.github_repository)
pull = repo.get_pull(args.pr)

# Remove old comment
for comment in pull.get_issue_comments():
if not comment.user.login == 'github-actions[bot]':
continue
if not comment.body.startswith(args.title):
continue
logging.info('Removing obsolete comment with heading "%s"', (args.title))
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a pretty sharp knife: if someone accidentally passes in empty string, this will delete all the comments, no?

I'd prefer it if all comments posted by this script had a known prefix from the script itself (which gets prepended to args.title).

Also, this is presumably copied in large part from bloat_check.py. How feasible would it be to make that script use this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me re-enable the user id check, to see if we can guard this one

comment.delete()

# Post new comment
pull.create_issue_comment("""{title} from {baseSha}

<details>
<summary>Full output</summary>

```
{content}
```

</details>
""".format(title=args.title, content=rawText, baseSha=pull.base.sha))

except Exception as e:
tb = traceback.format_exc()
logging.warning('Failed to post comment: %s', tb)



if __name__ == '__main__':
# execute only if run as a script
main()