SpenceKonde / megaTinyCore

Arduino core for the tinyAVR 0/1/2-series - Ones's digit 2,4,5,7 (pincount, 8,14,20,24), tens digit 0, 1, or 2 (featureset), preceded by flash in kb. Library maintainers: porting help available!
Other
549 stars 143 forks source link

Improve CI testing #306

Closed SpenceKonde closed 3 years ago

SpenceKonde commented 3 years ago

Two fronts to this:

  1. The current suite needs to be adapted to run tests with different millis timing sources:
  1. We need some additional examples to compile - ('d check them in under the megaTinyCore library, and note in comments that they're mainly for automated testing. What comes to mind is:

Could totally have the packaging script delete the examples that are purely for testing so they don't confuse and annoy - if there are complaints at least. I wouldn't go out of my way to hide them un;less someone whines.

per1234 commented 3 years ago

The current suite needs to be adapted to run tests with different millis timing sources:

When I started out on the project of setting up the sketch compilation CI workflow, my naive approach was to make a matrix to create a job for every possible combination of board vs board option. Unfortunately, because of the extreme awesomeness of this platform, it ends up multiplying up into something like hundreds of thousands of jobs when you do that! I found it very limiting to find a configuration that would fit within the GitHub Actions 256 jobs per workflow maximum limit. But I think there is definitely some room to add more jobs. Just giving you a heads up so you don't need to learn this the hard way as I did.

I'd check them in under the megaTinyCore library, and note in comments that they're mainly for automated testing.

You could put them outside the libraries folder (maybe under the extras folder) if you don't want to expose the user to them. That saves you from the effort of making the release script deal with them.

SpenceKonde commented 3 years ago

Thanks for the heads up.

I'm still trying to wrap my head around how to modify the matrix . When it's done automatically, as opposed to the manual list of ATTinyCore - yes, it's more graceful, but then there's a learning curve to modifying it.

(wouldn't the trick just be figuring out how to make an "action" encompass compiling with each clock option instead of a specific one, or something?)

As an aside, I don't think I've ever seen a clock-speed dependent failure that would show up on CI (lots of times where the sketches didn't run at the right speed, but I can't think of any times when a compile failed at one speed and not at others. Maybe for 16-derived vs 20-derived, and maybe if you cranked the clock down to 1 MHz so it tripped all the tests to change core behavior to work better with slow clock speed.

per1234 commented 3 years ago

Thanks for the heads up.

You're welcome. I just checked and it looks like the current configuration generates 50 jobs. So if there were 6 settings (looks like it varies) of the millis board option and you add that option into the matrix the 50 jobs becomes 300!

When it's done automatically, as opposed to the manual list of ATTinyCore - yes, it's more graceful, but then there's a learning curve to modifying it.

If you prefer the "wall of YAML" approach used in the ATTinyCore CI configuration, you could just define each of the matrix jobs individually rather than generating them programmatically as is currently done in this repo's "Compile Examples" workflow. Something like this:

name: Compile Examples

# See: https://docs.github.com/en/actions/reference/events-that-trigger-workflows
on:
  pull_request:
    paths:
      - ".github/workflows/compile-examples.yml"
      - "megaavr/libraries/**"
      - "megaavr/cores/**"
      - "megaavr/variants/**"
  push:
    paths:
      - ".github/workflows/compile-examples.yml"
      - "megaavr/libraries/**"
      - "megaavr/cores/**"
      - "megaavr/variants/**"
  workflow_dispatch:
  repository_dispatch:

jobs:
  compile-examples:
    runs-on: ubuntu-latest

    strategy:
      fail-fast: false

      matrix:
        job:
          - fqbn: megaTinyCore:megaavr:atxy7:chip=3217,millis=enabled
            sketch-paths: |
              # list of paths containing sketches to compile for this board
          # ~200 more job definitions here

    steps:
      - name: Checkout repository
        uses: actions/checkout@v2

      # See: https://github.com/arduino/compile-sketches/README.md
      - name: Compile examples
        uses: arduino/compile-sketches@main
        with:
          fqbn: ${{ matrix.job.fqbn }}
          sketch-paths: |
            ${{ matrix.job.sketch-paths }}
          platforms: |
            # Install megaTinyCore via Boards Manager for the toolchain
            - source-url: http://drazzy.com/package_drazzy.com_index.json
              name: ${{ env.platform-name }}
            # Overwrite the megaTinyCore release version with the platform from the local path
            - source-path: megaavr
              name: ${{ env.platform-name }}
          libraries: |
            # The sketches don't have any external library dependencies, so just define an empty array
            -
          verbose: false
          enable-deltas-report: true
          enable-warnings-report: true
          sketches-report-path: sketches-reports

      # The report artifact is the mechanism used to pass data to the Report Size Deltas workflow
      - name: Save memory usage change report as artifact
        uses: actions/upload-artifact@v2
        with:
          name: sketches-reports
          path: sketches-reports

wouldn't the trick just be figuring out how to make an "action" encompass compiling with each clock option instead of a specific one, or something?

The design of the arduino/compile-sketches action is based around compiling an unlimited number of sketches for a single board. This was already established before I took over the project, but I think it's a reasonable approach. This is actually how I did it in my arduino-ci-script project.

What you could do to compile for more than 256 boards is to run multiple arduino/compile-sketches steps per job. I haven't found anywhere that GitHub states a maximum number of steps.

I don't think I've ever seen a clock-speed dependent failure that would show up on CI

This is where I'm often at a disadvantage when setting up CI systems for software that I don't fully understand. It's obvious that a board option that only affects fuses and doesn't involve the code at all is a lower priority (though it's still nice to do at least a basic compilation just to verify the board definition is valid), but when it's something that changes the value of a macro I usually think it's a higher priority to give good coverage because I don't know what sort of conditional compilation might be based on that macro.

SpenceKonde commented 3 years ago

Say I was to fork the compile-sketches repo. add in two more options, millis_timer_list, and clock_option_list, pass them in as space separates lists just like sketch_paths and then in the start at line 39 I were to insert (this is copy-paste without understanding in significant part "well I need to do it with a list of options.... do they pass a list in any other way? Oh the list of sketches, ctrl+c ctrl+v)

millis_timer_list=os.environ["INPUT_TIMER-LIST"]
millis_timer_list = get_list_from_multiformat_input(input_value=millis_timer_list)
clock_option_list=os.environ["INPUT_CLOCK-LIST"]
clock_option_list= get_list_from_multiformat_input(input_value=clock_option_list)
for millis_timer in millis_timer_list:
  for clock_option in clock_option_list:
    (lines 40-41, with 2 extra levels of indentation)
        fqbn_arg=os.environ["INPUT_FQBN"]+":"+millis_timer+":"+clock_option,

Like I realize this is ugly arguably objectively the Wrong Way, but Im looking at this from the perspective of "how do I get effective test coverage from here in the minimum time possible, since I have a million things to do? DxCore and megaTinyCore have millis timers options, and all three have clock options. Heck, maybe these new arguments should have generic names, matrix_one_row, matrix_one_col (and it would do all of those combinations - for like, millis source and 20,16,5,4,1 MHz and each of the 2-5 timers ... and then you'd have non-matrix-options for the things that don't multiply with each other in terms of need for tet of all the permutations - ex on DxCore, how much of the app you mark as writable, or on the ATTinyCore, where more parts have multiple damned pin mappings than not, (which sucks! and 1 or 2 more get are getting one for 1.5.0, because I wanted to do something with them and realized that their pinmappings weren't fit for purpose. some of the pin mappings that people used back in the day were BEYOND BAD... and I didn't realize how bad they were because I was still noob back then) But yeah - the other side to the argument about whether modifying compile-sketches is appropriate is well, actually, as it stands with that limit on the number of jobs, compile-sketches isn't an ideal tool for testing, because you need to invoke it once per FQBN. I favor this over a multistep job for the simple reason that I know how to do this, and I don't have a clue about the other ways!

per1234 commented 3 years ago

Say I was to fork the compile-sketches repo. add in two more options, millis_timer_list, and clock_option_list, pass them in as space separates lists just like sketch_paths

I would do it by making the action's fqbn input accept a YAML document that is an array, just like how it is already done with the platforms, libraries, sketch-paths, and cli-compile-flags inputs:

      - name: Compile examples
        uses: arduino/compile-sketches@main
        with:
          fqbn: |
            - ${{ matrix.job.fqbn }}:millis=enabled
            - ${{ matrix.job.fqbn }}:millis=disabled
            - ${{ matrix.job.fqbn }}:millis=timera

That is consistent with the existing API and not at all hacky. There is already code in place in arduino/compile-sketches to support this in a backwards compatible manner. The only downside is that I think it will be a bit more tricky to programmatically adapt the list of board options according to each board if all boards don't have same list. It's certainly doable though, and I think you were wanting to step away from the programmatic job creation approach I used anyway.

compile-sketches isn't an ideal tool for testing, because you need to invoke it once per FQBN.

Keep in mind that, as I said in my last reply, even though arduino/compile-sketches only supports one FQBN per invocation, you are welcome to call it as many times as you like per job. So you can just copy that step multiple times and use a different custom board option in each step:

      # See: https://github.com/arduino/compile-sketches/README.md
      - name: Compile examples (millis=enabled)
        uses: arduino/compile-sketches@main
        with:
          fqbn: ${{ matrix.job.fqbn }}:millis=enabled
          sketch-paths: |
            ${{ matrix.job.sketch-paths }}
          platforms: |
            # Install megaTinyCore via Boards Manager for the toolchain
            - source-url: http://drazzy.com/package_drazzy.com_index.json
              name: ${{ env.platform-name }}
            # Overwrite the megaTinyCore release version with the platform from the local path
            - source-path: megaavr
              name: ${{ env.platform-name }}
          libraries: |
            # The sketches don't have any external library dependencies, so just define an empty array
            -
          verbose: false
          enable-deltas-report: true
          enable-warnings-report: true
          sketches-report-path: sketches-reports

      # See: https://github.com/arduino/compile-sketches/README.md
      - name: Compile examples (millis=disabled)
        uses: arduino/compile-sketches@main
        with:
          fqbn: ${{ matrix.job.fqbn }}:millis=disabled
          sketch-paths: |
            ${{ matrix.job.sketch-paths }}
          platforms: |
            # Install megaTinyCore via Boards Manager for the toolchain
            - source-url: http://drazzy.com/package_drazzy.com_index.json
              name: ${{ env.platform-name }}
            # Overwrite the megaTinyCore release version with the platform from the local path
            - source-path: megaavr
              name: ${{ env.platform-name }}
          libraries: |
            # The sketches don't have any external library dependencies, so just define an empty array
            -
          verbose: false
          enable-deltas-report: true
          enable-warnings-report: true
          sketches-report-path: sketches-reports

This might actually end up being easier to adjust to different board options being available for each board because you can make steps conditional: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepsif

I don't have a clue about the other ways!

Well, I've already said that I'm willing to help you out with this.

per1234 commented 3 years ago

I actually hit this maximum job limit when I switched my "wirino" platform to GitHub Actions last week. In case it's of interest, I'll share my solution: https://github.com/per1234/wirino/blob/master/.github/workflows/compile-sketches.yml#L52-L86 What I did was configure it to compile for each of the custom board options at least once. It doesn't cover all possible combinations of every option, but I think it provides reasonable test coverage.

SpenceKonde commented 3 years ago

So all I need to do is make copies of

- name: Compile examples
        uses: arduino/compile-sketches@main
        with:
          fqbn: ${{ env.platform-name }}:atxy${{ matrix.IO-class }}${{ matrix.bootloader-code }}:chip=${{ matrix.flash-class }}${{ matrix.peripheral-class }}${{ matrix.IO-class }}
          sketch-paths: |
            # It's necessary to jump through some hoops to dynamically generate the env object keys to define the non-universal sketch paths
            # https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions#format
            ${{ env[format('available-flash-7_5kB-plus-{0}-sketch-paths', matrix.available-flash-kB >= 7.5)] }}
            ${{ env[format('available-flash-4kB-plus-{0}-sketch-paths', matrix.available-flash-kB >= 4)] }}
            ${{ env[format('available-flash-3_5kB-plus-{0}-sketch-paths', matrix.available-flash-kB >= 3.5)] }}
            ${{ env[format('available-flash-2kB-plus-{0}-sketch-paths', matrix.available-flash-kB >= 2)] }}
            ${{ env[format('available-flash-1_5kB-plus-{0}-sketch-paths', matrix.available-flash-kB >= 1.5)] }}

            ${{ env[format('IO-class-4-plus-{0}-sketch-paths', matrix.IO-class >= 4)] }}
            ${{ env[format('IO-class-{0}-series-{1}-sketch-paths', matrix.IO-class, matrix.peripheral-class)] }}
          platforms: |
            # Install megaTinyCore via Boards Manager for the toolchain
            - source-url: http://drazzy.com/package_drazzy.com_index.json
              name: ${{ env.platform-name }}
            # Overwrite the megaTinyCore release version with the platform from the local path
            - source-path: megaavr
              name: ${{ env.platform-name }}
          libraries: |
            # The sketches don't have any external library dependencies, so just define an empty array
            -
          verbose: false
          enable-deltas-report: true
          enable-warnings-report: true
          sketches-report-path: sketches-reports

in that file, and make them specify which specify the millis and clock options?

That is a lot less work amd mess - and this testing is a lot more important now in 2.4.0 as we are adding a bunch of new complicated clock options (to support internal oscillators tuned to different speeds, after I realized just how far you can push the new 2-series parts. Those things will hit 32 @5v room temp running from the internal oscillator! 32 MHz internal isn't just for the DX-series anymore!

I also need to adapt it for the 2-series optiboot boards. Unfortunately I couldn't shoehorn the 2 and 0/1 into the same board definitions for Optiboot - the reset pin configuration menu needs to be too different, what with the whole alt reset pin option,. So those boards are named atx2(io class_)o - though the non optiboot boards don't have a separate entry. So non-opyiboot ones are all of txy2 txy4 txy6 txy7, 0/1-series optiboot are txy2o txy4o txy6o txy7o, and 2-series is tx24, tx26 and tx27. It seems awkward to adapt the matrix for that...

I've also started writing "compiletest" sketches under extras, which I think I can see how to add to the tests.

SpenceKonde commented 3 years ago

Okay, I am stuck on how to deal with the fact that the 2-series optiboot boards are atx2${{ matrix.IO-class }}${{ matrix.bootloader-code }} instead of atxy${{ matrix.IO-class }}${{ matrix.bootloader-code }}

SpenceKonde commented 3 years ago

I've added the first few test sketches for sanity checking, there are more to come obviously.

SpenceKonde commented 3 years ago

YAARRGGGHHHJH I trried making copies of the actions, but now I'm getting this error and I do not know how to solve it!

Traceback (most recent call last): File "/home/runner/work/_actions/arduino/compile-sketches/main/compilesketches/compilesketches.py", line 1709, in main() # pragma: no cover File "/home/runner/work/_actions/arduino/compile-sketches/main/compilesketches/compilesketches.py", line 55, in main compile_sketches.compile_sketches() File "/home/runner/work/_actions/arduino/compile-sketches/main/compilesketches/compilesketches.py", line 194, in compile_sketches self.install_arduino_cli() File "/home/runner/work/_actions/arduino/compile-sketches/main/compilesketches/compilesketches.py", line 231, in install_arduino_cli self.install_from_download( File "/home/runner/work/_actions/arduino/compile-sketches/main/compilesketches/compilesketches.py", line 290, in install_from_download self.install_from_path(source_path=absolute_source_path, File "/home/runner/work/_actions/arduino/compile-sketches/main/compilesketches/compilesketches.py", line 574, in install_from_path destination_path.symlink_to(target=source_path, target_is_directory=source_path.is_dir()) File "/usr/lib/python3.8/pathlib.py", line 1384, in symlink_to self._accessor.symlink(target, self, target_is_directory) File "/usr/lib/python3.8/pathlib.py", line 446, in symlink return os.symlink(a, b) FileExistsError: [Errno 17] File exists: '/tmp/compilesketches-erq15nhu/install_from_download-kgx51c3w/arduino-cli' -> '/home/runner/bin/arduino-cli' Error: Process completed with exit code 1.

@per1234 - do yu have any idea what this means or how to fix it? It seems like it's attempting to install things twice. Why? How do I keep that from happening?

SpenceKonde commented 3 years ago

Besides that and the issue with the 2-series optiboot board names, I think that's all that's standing in the way of my setting up real, rigorous proper CI testing here! Which is very exciting. Once it works here, I plan to roll it out to DxCore post-haste, and for ATTinyCore 2.0.0 as well.

With other pressing issues finally done, this is now priority #1 for megaTinyCore and DxCore.

SpenceKonde commented 3 years ago

Unless I figure out a solution tonight, 2,4.0 will be released without CI having been run with the new clock configurations. No more delays, this release needs to GTFO.

SpenceKonde commented 3 years ago

I'm so close, it looks like it chokes simply because of the presence of old temp files from the previous run, but I don't know how eityher make it skip that early setuip step that generates the duplicate file nor delete them at theend of each test case,

per1234 commented 3 years ago

Hi @SpenceKonde. I apologize for the trouble and for being so slow to respond.

do yu have any idea what this means or how to fix it?

Yes, you are right that it is failing due to leftover files from the previous run of the action. I need to figure out how best to make the action clean up after itself.

It is at least supposed to print a friendly error message when this happens, but it turns out that the code to check for a conflicting file doesn't work correctly in this particular case where the target of the symlink has been removed (because the target is in a temporary folder), The check is redirected by the symlink and reports back that the symlink target does not exist, but what is really needed is to know whether the symlink itself exists.

I'm going to try to work on this today. Until then, I can provide you with a workaround. Add this step to your workflow between each step that uses the arduino/compile-sketches action: https://github.com/per1234/megaTinyCore/commit/6f16bf387de271df18ffd8540be559cd7dba70f8

per1234 commented 3 years ago

As for the current version of the workflow file, this error: https://github.com/SpenceKonde/megaTinyCore/runs/3186986121?check_suite_focus=true#step:3:145

TypeError: 'NoneType' object is not iterable

is caused by having an empty platforms input here: https://github.com/SpenceKonde/megaTinyCore/actions/runs/1076714997/workflow#L194 This input is expected to contain a YAML document with an array type. If for some reason you don't want to install any platforms, you should provide an empty array:

      - uses: arduino/compile-sketches@main
        with:
          platforms: |
            []

This "don't install any platforms" use case isn't something I have considered, so I'm not entirely sure how well behaved the tool will be, but I would expect that it will simply try to compile the sketches. Assuming the platform dependencies of the sketch are already installed, it should compile.

SpenceKonde commented 3 years ago

Thanks much for help, hope to get it CIi-ng deeply tomorrow.

The current version is the result of me being likke "it's gotta be leftovers from the instal!! So I';ll see if I can make it not install anything on subsequent runs so it will work"

Beczuse I don't know jack fuckall a bout how this magic works behind the scenes, I stuff like that as my day job and I have comes to hate testing things with asn incredible passion, yet I am intimately familiar with how important it is (Maybe I should lower mty standards to more of a Microchip level - ya know, no big deal if there are 30 bugs known at release smany of them severe, and you find 15 more before you fix half a doze, you're doing fine)

Had the scare of scares earlier - suddenly, I couldn't program ANYTHING. Not with serizalUPDI, not with jtag2upid.... I was getting scared. Grabbed my old version - and it didn't work either... At which Ipoint I'm like wait a second that doesn't make sense.... Turns out the power cord on the self-powered USB hub had pulled out and vanished behind the desk, so rhe downstream hub was browning out. in a way that mimicked no response from the target.... Very weird, amd quirefrightening until I realized it was spurious. Need to hunt that power suppty conecctor and tape it on with gadffers tape too (already have tape to hold the keyboard dongle in. First night the sweet loving grey kitty slept in my room, she woke up in the middle of the night, and attacked the dongle, ripping it from the hub and destroying it - it was a $30 kb, no dongle replacement available that I had bought that week. And like, she nevwer does stuff like that! I broke Tone somehow too that I need to fix before release, too.

per1234 commented 3 years ago

OK, I think the action should properly support being run multiple times in the same workflow job now: (https://github.com/arduino/compile-sketches/pull/28, https://github.com/arduino/compile-sketches/pull/29) Please let me know if you run into any problems.

I'm glad to hear the programmer glitch turned out to be innocuous in the end. I think that troubleshooting in embedded systems tends to have more bisection axes than PC application development. As for kitty troubleshooting, I it is surely an arcane art.

SpenceKonde commented 3 years ago

Oh sick, so I don't gotta do shit to fix my tests? This is my favorite kind of issue!

per1234 commented 3 years ago

I think so. Here is the run I did with the workflow as it was at https://github.com/SpenceKonde/megaTinyCore/commit/890b8ede2683d446188a604168d084de4f584f25 https://github.com/per1234/megaTinyCore/runs/3211482563 I didn't look super closely to interpret the results, but it seems OK at a glance.

SpenceKonde commented 3 years ago

Okay. Well.... I clearly broke somethingL

What did I do wrong...

Running command: /home/runner/bin/arduino-cli core install megaTinyCore:megaavr 
   Error during install: finding platform dependencies: package megaTinyCore not found

Error: Command failed
Error: Process completed with exit code 1.
per1234 commented 3 years ago

The platforms input is missing from the first two arduino/compile-sketches steps of the workflow. When no platform dependencies are defined, the action gets it from the first part of the FQBN. That works for official boards, since the packages from https://downloads.arduino.cc/packages/package_index.json are always accessible, but 3rd party platforms require the package index URL to be specified (e.g., http://drazzy.com/package_drazzy.com_index.json), so it fails when it attempts to install the platform.

I see the later workflow steps do have the platforms input: https://github.com/SpenceKonde/megaTinyCore/blob/5c1e981df1667122263fff7e61112c64029471e3/.github/workflows/compile-examples.yml#L238-L244

SpenceKonde commented 3 years ago

Ugh how silly. Forgot to undo that.

Also your more ssuccessful run turned up a legit bug in the tinyNeoPixel libraries.

Now the only thing I need to figure out is how to add 5 new board definitions, atx24o atx26o, atx27o (optiboot version for 2-series; needed to restructure the updi pin configuration menu to prevent user confusion. but I think the CI testing will be nearly identical. (just have concerns over a few minor points plus microchip and microchipo (these are the official microchip boards,. with LED_BUILTIN on the correct pin, PIN_BUTTON_BUILTIN for the button, and "upload" will always use the onboard debugger. for non-optiboot)). would definitely be nice to be able to add those to the matrix, and I'm sure it would make the Microchip folks happy :-)

SpenceKonde commented 3 years ago

Okay, let's fix all of them and try that again

Looks like github is borked currently, tons of runs are failing because of timeouts connecting to github,,,,

per1234 commented 3 years ago

Also your more ssuccessful run turned up a legit bug in the tinyNeoPixel libraries.

I'm glad to hear that your efforts to expand the CI coverage are already paying off!

Looks like github is borked currently, tons of runs are failing because of timeouts connecting to github,,,,

I'm also seeing run failures. I get notifications for workflow run failures in a large number of repositories that have scheduled workflow runs, so my email inbox ends up being an indicator of the health of GitHub's systems.

This is unfortunately a pretty common occurrence these days. GitHub Actions is very integral to the development workflow of many of the projects I'm involved with so it can really put a wrench in the works. Frustrating, but Travis CI had the same issues. Fortunately, GitHub does usually get things back to normal within a couple hours

SpenceKonde commented 3 years ago

I may actually have the tests running correctly now!..

SpenceKonde commented 3 years ago

CI tests seem to be mostly working now, just need to work out how to make one of the test cases only run on 1/2-series