From 3c8712c5c5bf2e7d1efd72480aa086c128adfa84 Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 10:38:27 +0200 Subject: [PATCH 1/9] add new workflow with PR comparison --- .github/workflows/main.yml | 103 +++++++++++++++++++++----- tools/compare_maps.py | 146 +++++++++++++++++++++++++++++++++++++ 2 files changed, 231 insertions(+), 18 deletions(-) create mode 100644 tools/compare_maps.py diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 4a71b7e08e..20048db95f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -11,11 +11,11 @@ on: # Run this Workflow when files are updated (Pushed) in the "master" Branch push: - branches: [ master ] + branches: [ master, develop ] # Also run this Workflow when a Pull Request is created or updated in the "master" Branch pull_request: - branches: [ master ] + branches: [ master, develop ] # Steps to run for the Workflow jobs: @@ -45,6 +45,8 @@ jobs: - name: Install Embedded Arm Toolchain arm-none-eabi-gcc if: steps.cache-toolchain.outputs.cache-hit != 'true' # Install toolchain if not found in cache uses: fiam/arm-none-eabi-gcc@v1.0.2 + env: + ACTIONS_ALLOW_UNSECURE_COMMANDS: true with: # GNU Embedded Toolchain for Arm release name, in the V-YYYY-qZ format (e.g. "9-2019-q4") release: 9-2020-q2 @@ -86,19 +88,24 @@ jobs: git clone --branch v1.5.0 https://github.com/JuulLabs-OSS/mcuboot - name: Install imgtool dependencies - run: pip3 install --user -r ${{ runner.temp }}/mcuboot/scripts/requirements.txt + run: | + cat ${{ runner.temp }}/mcuboot/scripts/requirements.txt + pip3 install --user -r ${{ runner.temp }}/mcuboot/scripts/requirements.txt - name: Install adafruit-nrfutil run: | pip3 install --user wheel pip3 install --user setuptools pip3 install --user adafruit-nrfutil + pip3 install --user 'cbor>=1.0.0' ######################################################################################### # Checkout - name: Checkout source files uses: actions/checkout@v2 + with: + submodules: true - name: Show files run: set ; pwd ; ls -l @@ -112,29 +119,22 @@ jobs: cd build cmake -DARM_NONE_EABI_TOOLCHAIN_PATH=${{ runner.temp }}/arm-none-eabi -DNRF5_SDK_PATH=${{ runner.temp }}/nrf5_sdk -DUSE_OPENOCD=1 ../ - ######################################################################################### - # Make and Upload DFU Package - # pinetime-mcuboot-app.img must be flashed at address 0x8000 in the internal flash memory with OpenOCD: - # program image.bin 0x8000 + ######################################################################################### + # Make and Upload DFU Package + # pinetime-mcuboot-app.img must be flashed at address 0x8000 in the internal flash memory with OpenOCD: + # program image.bin 0x8000 - # For Debugging Builds: Remove "make" option "-j" for clearer output. Add "--trace" to see details. - # For Faster Builds: Add "make" option "-j" + # For Debugging Builds: Remove "make" option "-j" for clearer output. Add "--trace" to see details. + # For Faster Builds: Add "make" option "-j" - name: Make pinetime-mcuboot-app run: | cd build make pinetime-mcuboot-app - - name: Create firmware image - run: | - # The generated firmware binary looks like "pinetime-mcuboot-app-0.8.2.bin" - ls -l build/src/pinetime-mcuboot-app*.bin - ${{ runner.temp }}/mcuboot/scripts/imgtool.py create --align 4 --version 1.0.0 --header-size 32 --slot-size 475136 --pad-header build/src/pinetime-mcuboot-app*.bin build/src/pinetime-mcuboot-app-img.bin - ${{ runner.temp }}/mcuboot/scripts/imgtool.py verify build/src/pinetime-mcuboot-app-img.bin - - name: Create DFU package run: | - ~/.local/bin/adafruit-nrfutil dfu genpkg --dev-type 0x0052 --application build/src/pinetime-mcuboot-app-img.bin build/src/pinetime-mcuboot-app-dfu.zip + ~/.local/bin/adafruit-nrfutil dfu genpkg --dev-type 0x0052 --application build/src/pinetime-mcuboot-app-image-1.0.0.bin build/src/pinetime-mcuboot-app-dfu.zip unzip -v build/src/pinetime-mcuboot-app-dfu.zip # Unzip the package because Upload Artifact will zip up the files unzip build/src/pinetime-mcuboot-app-dfu.zip -d build/src/pinetime-mcuboot-app-dfu @@ -150,8 +150,10 @@ jobs: - name: Make pinetime-app run: | + mkdir -p maps/ cd build - make pinetime-app + make -j pinetime-app + cp src/pinetime-app-1.0.0.map ../maps/proposed.map - name: Upload standalone firmware uses: actions/upload-artifact@v2 @@ -166,6 +168,71 @@ jobs: run: | find . -name "pinetime-app.*" -ls find . -name "pinetime-mcuboot-app.*" -ls + + ######################################################################################### + # PR related tasks + - name: Get map for target branch + uses: actions/cache@v2 + id: cache-map + if: github.event_name == 'pull_request' + with: + path: maps/${{ github.event.pull_request.base.sha }}.map + key: map-${{ github.event.pull_request.base.sha }} + + + - name: Checkout PR target files + uses: actions/checkout@v2 + if: steps.cache-map.outputs.cache-hit != 'true' && github.event_name == 'pull_request' + with: + # using sha instead of ref as different points in time will have different map files + ref: ${{ github.event.pull_request.base.sha }} + submodules: true + + - name: CMake + if: steps.cache-map.outputs.cache-hit != 'true' && github.event_name == 'pull_request' + run: | + rm -rf build + mkdir -p build maps + cd build + cmake -DARM_NONE_EABI_TOOLCHAIN_PATH=${{ runner.temp }}/arm-none-eabi -DNRF5_SDK_PATH=${{ runner.temp }}/nrf5_sdk -DUSE_OPENOCD=1 ../ + + ######################################################################################### + # Make and Upload DFU Package + # pinetime-mcuboot-app.img must be flashed at address 0x8000 in the internal flash memory with OpenOCD: + # program image.bin 0x8000 + + # For Debugging Builds: Remove "make" option "-j" for clearer output. Add "--trace" to see details. + # For Faster Builds: Add "make" option "-j" + + - name: Make pinetime-app + if: steps.cache-map.outputs.cache-hit != 'true' && github.event_name == 'pull_request' + run: | + cd build + make -j pinetime-app + cp src/pinetime-app-1.0.0.map ../maps/${{ github.event.pull_request.base.sha }}.map + + - name: Compare map versions + run: | + python3 tools/compare_maps.py maps/${{ github.event.pull_request.base.sha }}.map maps/proposed.map + echo 'CHANGES_TABLE<> $GITHUB_ENV + python3 tools/compare_maps.py maps/${{ github.event.pull_request.base.sha }}.map maps/proposed.map >> $GITHUB_ENV + echo 'EOF' >> $GITHUB_ENV + + - uses: actions/github-script@v4 + if: github.event_name == 'pull_request' + with: + github-token: ${{secrets.GITHUB_TOKEN}} + script: | + const { CHANGES_TABLE } = process.env; + if (CHANGES_TABLE.trim() !== '') { + github.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: CHANGES_TABLE, + }); + } + # Embedded Arm Toolchain and nRF5 SDK will only be cached if the build succeeds. # So make sure that the first build always succeeds, e.g. comment out the "Make" step. diff --git a/tools/compare_maps.py b/tools/compare_maps.py new file mode 100644 index 0000000000..5ffd603ef8 --- /dev/null +++ b/tools/compare_maps.py @@ -0,0 +1,146 @@ +# vim: set fileencoding=utf8 : + +import argparse +import re, os +from itertools import groupby + +class Objectfile: + def __init__ (self, section, offset, size, comment): + self.section = section.strip () + self.offset = offset + self.size = size + self.path = (None, None) + self.basepath = None + if comment: + self.path = re.match (r'^(.+?)(?:\(([^\)]+)\))?$', comment).groups () + self.basepath = os.path.basename (self.path[0]) + self.children = [] + + def __repr__ (self): + return ''.format (self.section, self.offset, self.size, self.path, repr (self.children)) + +def parseSections (fd): + """ + Quick&Dirty parsing for GNU ld’s linker map output, needs LANG=C, because + some messages are localized. + """ + + sections = [] + + # skip until memory map is found + found = False + while True: + l = fd.readline () + if not l: + break + if l.strip () == 'Memory Configuration': + found = True + break + if not found: + return None + + # long section names result in a linebreak afterwards + sectionre = re.compile ('(?P
.+?|.{14,}\n)[ ]+0x(?P[0-9a-f]+)[ ]+0x(?P[0-9a-f]+)(?:[ ]+(?P.+))?\n+', re.I) + subsectionre = re.compile ('[ ]{16}0x(?P[0-9a-f]+)[ ]+(?P.+)\n+', re.I) + s = fd.read () + pos = 0 + while True: + m = sectionre.match (s, pos) + if not m: + # skip that line + try: + nextpos = s.index ('\n', pos)+1 + pos = nextpos + continue + except ValueError: + break + pos = m.end () + section = m.group ('section') + v = m.group ('offset') + offset = int (v, 16) if v is not None else None + v = m.group ('size') + size = int (v, 16) if v is not None else None + comment = m.group ('comment') + if section != '*default*' and size > 0: + of = Objectfile (section, offset, size, comment) + if section.startswith (' '): + sections[-1].children.append (of) + while True: + m = subsectionre.match (s, pos) + if not m: + break + pos = m.end () + offset, function = m.groups () + offset = int (offset, 16) + if sections and sections[-1].children: + sections[-1].children[-1].children.append ((offset, function)) + else: + sections.append (of) + + return sections + +def measure_map(fname): + sections = parseSections(open(fname, 'r')) + if sections is None: + print ('start of memory config not found, did you invoke the compiler/linker with LANG=C?') + return + + sectionWhitelist = {'.text', '.data', '.bss', '.rodata'} + whitelistedSections = list (filter (lambda x: x.section in sectionWhitelist, sections)) + grouped_objects = {} + for s in whitelistedSections: + for k, g in groupby (sorted (s.children, key=lambda x: x.basepath), lambda x: x.basepath): + size = sum (map (lambda x: x.size, g)) + grouped_objects[k] = size + return grouped_objects + +def main(old_fname, new_fname): + old = measure_map(old_fname) + new = measure_map(new_fname) + total_new = sum(new.values()) + total_old = sum(old.values()) + diff = total_new-total_old + sign = "+" if diff > 0 else "-" + + if diff == 0: + return + + new_keys = new.keys() - old.keys() + shared_keys = set(new.keys()).intersection(old.keys()) + old_keys = old.keys() - new.keys() + + diffs = [] + for k in shared_keys: + delta = new[k] - old[k] + if delta == 0: + continue + diffs.append((delta, k)) + + for k in new_keys: + diffs.append((new[k], k)) + + for k in old_keys: + diffs.append((-old[k], k)) + + if diffs: + print('Object|Change (bytes)') + print('------|--------------') + for change, obj in sorted(diffs): + sign = "+" if change > 0 else "-" + print(f'{obj}|{sign}{abs(change)}') + + print(f'Total|{sign}{abs(diff)}') + +def parse_args(): + parser = argparse.ArgumentParser(description="Compare the respective sizes in 2 map files", epilog=''' + Example: + + compare_maps.py old.map new.map + ''') + parser.add_argument("old") + parser.add_argument("new") + args = parser.parse_args() + main(args.old, args.new) + +if __name__ == '__main__': + parse_args() From a0eeb8d895f8359ce03193953b107335b915feb3 Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 10:54:30 +0200 Subject: [PATCH 2/9] debug --- .github/workflows/main.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 20048db95f..8f2cc4767c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -150,9 +150,9 @@ jobs: - name: Make pinetime-app run: | - mkdir -p maps/ cd build make -j pinetime-app + ls -lh src cp src/pinetime-app-1.0.0.map ../maps/proposed.map - name: Upload standalone firmware @@ -179,7 +179,6 @@ jobs: path: maps/${{ github.event.pull_request.base.sha }}.map key: map-${{ github.event.pull_request.base.sha }} - - name: Checkout PR target files uses: actions/checkout@v2 if: steps.cache-map.outputs.cache-hit != 'true' && github.event_name == 'pull_request' From a076935a886526c6d2fa1b3b60d5436ca9ad17fe Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 11:01:50 +0200 Subject: [PATCH 3/9] mkdir maps --- .github/workflows/main.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 8f2cc4767c..5b1859cfc3 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -153,6 +153,7 @@ jobs: cd build make -j pinetime-app ls -lh src + mkdir -p ../maps cp src/pinetime-app-1.0.0.map ../maps/proposed.map - name: Upload standalone firmware From 6394f2ea6af67513e0b6637303bbc2b8cd75cb93 Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 12:07:14 +0200 Subject: [PATCH 4/9] avoid cleaning when checking out target --- .github/workflows/main.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5b1859cfc3..0f3223ce9f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -187,6 +187,7 @@ jobs: # using sha instead of ref as different points in time will have different map files ref: ${{ github.event.pull_request.base.sha }} submodules: true + clean: false - name: CMake if: steps.cache-map.outputs.cache-hit != 'true' && github.event_name == 'pull_request' From 1997ab2225a2cf570be9ba5eafca606e55409344 Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 12:26:41 +0200 Subject: [PATCH 5/9] align size to the right --- tools/compare_maps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/compare_maps.py b/tools/compare_maps.py index 5ffd603ef8..fc7aa58a50 100644 --- a/tools/compare_maps.py +++ b/tools/compare_maps.py @@ -124,7 +124,7 @@ def main(old_fname, new_fname): if diffs: print('Object|Change (bytes)') - print('------|--------------') + print('|:----|------------:|') for change, obj in sorted(diffs): sign = "+" if change > 0 else "-" print(f'{obj}|{sign}{abs(change)}') From 0ddb7b91458e3e527d0f88f7d23a76469e4fcaf5 Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 12:27:13 +0200 Subject: [PATCH 6/9] align size to the right --- tools/compare_maps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/compare_maps.py b/tools/compare_maps.py index fc7aa58a50..15ac4bbf6b 100644 --- a/tools/compare_maps.py +++ b/tools/compare_maps.py @@ -129,7 +129,7 @@ def main(old_fname, new_fname): sign = "+" if change > 0 else "-" print(f'{obj}|{sign}{abs(change)}') - print(f'Total|{sign}{abs(diff)}') + print(f'**Total**|{sign}{abs(diff)}') def parse_args(): parser = argparse.ArgumentParser(description="Compare the respective sizes in 2 map files", epilog=''' From eb9e44c8a1428894eee5f0b1c18a08ef8930471f Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 21:42:29 +0200 Subject: [PATCH 7/9] re-check-out repo to get scripts usable --- .github/workflows/main.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0f3223ce9f..193c8fa899 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -212,6 +212,12 @@ jobs: make -j pinetime-app cp src/pinetime-app-1.0.0.map ../maps/${{ github.event.pull_request.base.sha }}.map + - name: Checkout repo again + uses: actions/checkout@v2 + with: + submodules: true + clean: false + - name: Compare map versions run: | python3 tools/compare_maps.py maps/${{ github.event.pull_request.base.sha }}.map maps/proposed.map From 5bc8780ea4222ee507b32d151a8f218e4ce65697 Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 21:46:13 +0200 Subject: [PATCH 8/9] fix sign for total --- tools/compare_maps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/compare_maps.py b/tools/compare_maps.py index 15ac4bbf6b..023ba272f7 100644 --- a/tools/compare_maps.py +++ b/tools/compare_maps.py @@ -100,7 +100,6 @@ def main(old_fname, new_fname): total_new = sum(new.values()) total_old = sum(old.values()) diff = total_new-total_old - sign = "+" if diff > 0 else "-" if diff == 0: return @@ -129,6 +128,7 @@ def main(old_fname, new_fname): sign = "+" if change > 0 else "-" print(f'{obj}|{sign}{abs(change)}') + sign = "+" if diff > 0 else "-" print(f'**Total**|{sign}{abs(diff)}') def parse_args(): From ef22f995e4cc3b013beb7a849a388a4d2e34bb5e Mon Sep 17 00:00:00 2001 From: david Date: Wed, 5 May 2021 22:19:30 +0200 Subject: [PATCH 9/9] parse all sections --- tools/compare_maps.py | 49 ++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tools/compare_maps.py b/tools/compare_maps.py index 023ba272f7..244163262d 100644 --- a/tools/compare_maps.py +++ b/tools/compare_maps.py @@ -87,39 +87,40 @@ def measure_map(fname): sectionWhitelist = {'.text', '.data', '.bss', '.rodata'} whitelistedSections = list (filter (lambda x: x.section in sectionWhitelist, sections)) - grouped_objects = {} + grouped_objects = {s: {} for s in sectionWhitelist} for s in whitelistedSections: for k, g in groupby (sorted (s.children, key=lambda x: x.basepath), lambda x: x.basepath): size = sum (map (lambda x: x.size, g)) - grouped_objects[k] = size + grouped_objects[s.section][k] = size return grouped_objects def main(old_fname, new_fname): - old = measure_map(old_fname) - new = measure_map(new_fname) - total_new = sum(new.values()) - total_old = sum(old.values()) - diff = total_new-total_old - - if diff == 0: - return - - new_keys = new.keys() - old.keys() - shared_keys = set(new.keys()).intersection(old.keys()) - old_keys = old.keys() - new.keys() + old_by_section = measure_map(old_fname) + new_by_section = measure_map(new_fname) diffs = [] - for k in shared_keys: - delta = new[k] - old[k] - if delta == 0: + total_diff = 0 + for section in old_by_section.keys(): # sections are on both + new = new_by_section[section] + old = old_by_section[section] + total_new = sum(new.values()) + total_old = sum(old.values()) + diff = total_new-total_old + total_diff += diff + + if diff == 0: continue - diffs.append((delta, k)) - for k in new_keys: - diffs.append((new[k], k)) + all_keys = set(new.keys()).union(old.keys()) - for k in old_keys: - diffs.append((-old[k], k)) + for k in all_keys: + delta = new.get(k, 0) - old.get(k, 0) + if delta == 0: + continue + name = k + if section != '.text': + name = f'({section}){name}' + diffs.append((delta, name)) if diffs: print('Object|Change (bytes)') @@ -128,8 +129,8 @@ def main(old_fname, new_fname): sign = "+" if change > 0 else "-" print(f'{obj}|{sign}{abs(change)}') - sign = "+" if diff > 0 else "-" - print(f'**Total**|{sign}{abs(diff)}') + sign = "+" if total_diff > 0 else "-" + print(f'**Total**|{sign}{abs(total_diff)}') def parse_args(): parser = argparse.ArgumentParser(description="Compare the respective sizes in 2 map files", epilog='''