Skip to content

github: Move windows CI builds to GitHub Actions. #10263

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

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

dlech
Copy link
Contributor

@dlech dlech commented Dec 18, 2022

By moving to GitHub actions, all MicroPython CI builds are now on GitHub actions. This allows faster parallel builds and saves time by not building when no relevant files changed.

This reveals a few failing tests, so those are temporarily disabled until they can be fixed.

The misc_sys_settrace_features.py is disable on visual studio build because it is failing with:

--- "D:/a/micropython/micropython/tests/results\\misc_sys_settrace_features.py.exp"	2022-12-17 23:24:57.940564100 +0000
+++ "D:/a/micropython/micropython/tests/results\\misc_sys_settrace_features.py.out"	2022-12-17 23:24:57.940564100 +0000
@@ -40,799 +40,17 @@
  1:   @__main__:factorial => sys_settrace_features.py:91
  2:   @__main__:factorial => sys_settrace_features.py:91
  3:   @__main__:do_tests => sys_settrace_features.py:97
- 4:   @__main__:<module> => sys_settrace_features.py:106
-### trace_handler::main event: line
- 0:   @__main__:factorial => sys_settrace_features.py:88
- 1:   @__main__:factorial => sys_settrace_features.py:91
- 2:   @__main__:factorial => sys_settrace_features.py:91
- 3:   @__main__:do_tests => sys_settrace_features.py:97
- 4:   @__main__:<module> => sys_settrace_features.py:106
[snipped repeating]

------------------- script exited ------------------
-Total traces executed:  155
+Traceback (most recent call last):
+  File "misc/sys_settrace_features.py", line 106, in <module>
+  File "misc/sys_settrace_features.py", line 97, in do_tests
+  File "misc/sys_settrace_features.py", line 91, in factorial
+  File "misc/sys_settrace_features.py", line 91, in factorial
+  File "misc/sys_settrace_features.py", line 69, in trace_tick_handler
+  File "misc/sys_settrace_features.py", line 38, in trace_tick
+  File "misc/sys_settrace_features.py", line 30, in print_stacktrace
+  File "misc/sys_settrace_features.py", line 30, in print_stacktrace
+  File "misc/sys_settrace_features.py", line 30, in print_stacktrace
+  File "misc/sys_settrace_features.py", line 30, in print_stacktrace
+  File "misc/sys_settrace_features.py", line 25, in print_stacktrace
+RuntimeError: maximum recursion depth exceeded
+CRASH
\ No newline at end of file

FAILURE D:/a/micropython/micropython/tests/results\misc_sys_settrace_features.py

I think we didn't see this one before because Appveyor builds only did a Debug build for the standard variant but now we are only doing Debug build for the dev variant.

The import/import_file.py is disabled on msys builds because it is failing with:

 --- /d/a/micropython/micropython/tests/results/import_import_file.py.exp	2022-12-18 00:30:45.784368200 +0000
+++ /d/a/micropython/micropython/tests/results/import_import_file.py.out	2022-12-18 00:30:45.784368200 +0000
@@ -1 +1 @@
-D:\a\micropython\micropython\tests\import\import1b.py
+D:/a/micropython/micropython/tests/import/import1b.py

FAILURE /d/a/micropython/micropython/tests/results/import_import_file.py

On the bright side, the failing tests show that the conditional "Print failures" work as expected.

@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@dlech dlech force-pushed the windows-to-github-actions branch from 913c5b6 to 23e9179 Compare December 18, 2022 04:52
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@jimmo
Copy link
Member

jimmo commented Dec 18, 2022

Thanks @dlech this sounds really good. How easy is it to debug failing builds on thr GH Actions Windows infrastructure? One of the near features of Appveyor is that you could RDP into the VM for an hour after the build ran, which was quite handy for scenarios that were difficult to repro locally.

exclude:
- variant: standard
configuration: Debug
runs-on: windows-2022
Copy link
Contributor

Choose a reason for hiding this comment

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

The 2022 probably means VS2022 right? Appveyor was still running VS2017 intentionally to make sure we're somewhat backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, VS2022 is what is installed on this image by default. We can update this to install an older version if needed or even add multiple versions of VS to the build matrix.

Copy link
Member

Choose a reason for hiding this comment

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

I see that this PR's been updated to test 2017, 2019 and 2022. That's great!

@dlech
Copy link
Contributor Author

dlech commented Dec 18, 2022

How easy is it to debug failing builds on the GH Actions Windows infrastructure?

The same as Linux I guess. I've always been able to reproduce issues locally so I've never look any farther into it.

@dlech dlech force-pushed the windows-to-github-actions branch 4 times, most recently from b517bf9 to fe0bfbb Compare December 18, 2022 17:55
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

1 similar comment
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@dlech dlech force-pushed the windows-to-github-actions branch 2 times, most recently from 8f60789 to 034a298 Compare December 18, 2022 18:07
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@dlech dlech force-pushed the windows-to-github-actions branch from 034a298 to 5e2433d Compare December 18, 2022 18:24
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@dlech dlech force-pushed the windows-to-github-actions branch from 5e2433d to 05b72fc Compare December 18, 2022 18:39
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@dlech dlech force-pushed the windows-to-github-actions branch from 05b72fc to 9515f6c Compare December 18, 2022 20:37
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@dlech dlech force-pushed the windows-to-github-actions branch 2 times, most recently from 93f3ede to 14f33e7 Compare December 19, 2022 03:05
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@github-actions
Copy link

github-actions bot commented Dec 19, 2022

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@jimmo
Copy link
Member

jimmo commented Dec 19, 2022

How easy is it to debug failing builds on the GH Actions Windows infrastructure?

The same as Linux I guess. I've always been able to reproduce issues locally so I've never look any farther into it.

Generally I haven't had too much trouble reproducing Linux issues remotely (and with Docker I've been able to very easily replicate the Actions environment).

But for Windows this just isn't an option. I do (some of the time) have access to a Windows laptop, but certainly not one with all the different VS versions installed etc. So being able to RDP to the Appveyor instance has been invaluable.

@dlech
Copy link
Contributor Author

dlech commented Dec 19, 2022

Seems like it is at least technically possible to do something like this with github actions: https://medium.com/the-scale-factory/troubleshoot-github-actions-via-vpn-3d5d41b01ea4

choco install windows-sdk-8.1
- uses: microsoft/setup-msbuild@v1
with:
vs-version: ${{ matrix.vs_version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my information: are these spaces in {{}} required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

- uses: actions/setup-python@v4
if: matrix.runner == 'windows-2019'
with:
python-version: '3.9'
Copy link
Contributor

Choose a reason for hiding this comment

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

Appveyor used to have 3.8, any reason for 3.9 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It matches the version installed by default in the windows-2022 image.

@stinos
Copy link
Contributor

stinos commented Dec 19, 2022

Overall I like the idea of github actions for everything, but I also think we need at least a POC that remote login is possible. Or even a script on the wiki or so. Especially for mingw builds I've had it more than once that locally things worked ok but then failed on Appveyor because of a slightly different version or so. In which case RDP is usually quicker than trying to reproduce the exact same environment. An alternative would be to make it easier to reproduce the same environment I guess. Maybe a step which prints out versions of everything used explicitly (mingw/gcc/python) ?

configuration: Debug
runs-on: ${{ matrix.runner }}
steps:
- name: Install Visual Studio 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

This is rather slow, could this be cached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing that the installers poke windows registry, so probably not. Maybe there is a way to create a docker container with this pre-installed or something like that?

(It's already still faster than the current appveyor build though)

@dlech
Copy link
Contributor Author

dlech commented Jan 4, 2023

but I also think we need at least a POC that remote login is possible.

I was able to successfully log in via RDP with https://github.com/marketplace/actions/debugging-using-reverse-rdp-with-ngrok

Since it requires a personal access token and only allows one session at a time, I don't think it makes sense to commit anything here, but to use it. The basic procedure is to delete all of the other workflow files and all of the jobs except for one in the windows workflow file (so there is only one matrix combination left) and add this action as a step wherever it makes sense (may want it before or after other steps depending on where the failure is).

Also, don't do what I did and set the step to foreground: true and then shut down the remote windows vm without deleting Delete-This-File-To-Continue.txt. It caused the CI to hang and I couldn't start another run for a while (it eventual canceled).

@dpgeorge
Copy link
Member

I'd like to move forward with this PR and get it merged.

@stinos @jimmo do you have any objections given that it is possible to RDP (see above comment)?

@dpgeorge
Copy link
Member

Will need to merge #10267 before this one.

ifeq ($(MSYSTEM),MINGW32)
# compliler optimizations break float formatting
# https://github.com/micropython/micropython/pull/10267
RUN_TESTS_SKIP += -e format_ints_doubleprec -e format_modulo2_intbig
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to do the skipping here instead of directly in run_tests.py?

Copy link
Member

Choose a reason for hiding this comment

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

Merging #10267 will make this change here go away.

@stinos
Copy link
Contributor

stinos commented Jan 31, 2024

I gguess this should be fine. Might also make it easier to publish the executables?

@jimmo
Copy link
Member

jimmo commented Feb 5, 2024

@dpgeorge Yep keen to merge! Thanks @dlech

@dpgeorge dpgeorge force-pushed the windows-to-github-actions branch from 14f33e7 to 58bf8bf Compare February 5, 2024 03:29
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (23342ef) 98.36% compared to head (14f33e7) 98.49%.

❗ Current head 14f33e7 differs from pull request most recent head 58bf8bf. Consider uploading reports for the commit 58bf8bf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10263      +/-   ##
==========================================
+ Coverage   98.36%   98.49%   +0.13%     
==========================================
  Files         159      155       -4     
  Lines       21093    20528     -565     
==========================================
- Hits        20748    20220     -528     
+ Misses        345      308      -37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dlech
Copy link
Contributor Author

dlech commented Feb 5, 2024

It looks like the 2022 builds are failing because of an extra space in the path in the "Get micropython.exe path" step.

micropython= D:\a\micropython\micropython\ports\windows\build-dev\Releasex64\micropython.exe

There should not be a space between "=" and "D". Since the other VS version builds aren't failing, I'm guessing something must have changed in VS 2022 in the last year since this was working before. I'm pretty sure that is why I put the .Trim() call in there, so I'm not sure why we would end up with a space there.

@dlech
Copy link
Contributor Author

dlech commented Feb 5, 2024

I suppose it could have something to do with a different PowerShell version on the windows-2022 VM too.

@dpgeorge
Copy link
Member

dpgeorge commented Feb 5, 2024

Yes, I'm just trying to fix this now, with the space/Trim.

@dlech sorry but I forced pushed to your branch! I also rebased on latest master.

@dpgeorge dpgeorge force-pushed the windows-to-github-actions branch from 4b72696 to e015b8b Compare February 5, 2024 05:24
@dpgeorge
Copy link
Member

dpgeorge commented Feb 5, 2024

OK, I think this is working now. Needed a small tweak to remove the space between the variable name and the variable value.

This new github actions workflow has a lot of jobs... that's mainly due to a large job matrix with different versions of Visual Studio, x86 and x64, debug and release builds.

The VS 2017 jobs are very slow, they seem to get stuck on choco install visualstudio2017-workload-vctools for a while, and the whole "install visual studio" step takes about 6 minutes for VS 2017 (2019 and 2022 don't need this extra install step). Do we need to test against VS 2017? I'm guessing we do because that's what the current AppVeyor uses.

@dpgeorge dpgeorge force-pushed the windows-to-github-actions branch from 3328e36 to e015b8b Compare February 5, 2024 06:56
@dpgeorge
Copy link
Member

dpgeorge commented Feb 5, 2024

Maybe we can just exclude VS 2017 builds of the standard variant? That will cut off 2 jobs. Then there's still 4 jobs for VS 2017, all building the dev variant: x86 debug, x86 release, x64 debug, x64 release.

@stinos
Copy link
Contributor

stinos commented Feb 5, 2024

OK, I think this is working now. Needed a small tweak to remove the space between the variable name and the variable value.

I would think this is the issue described here yet you fixed this with only one Trim so not sure.

This new github actions workflow has a lot of jobs... that's mainly due to a large job matrix with different versions of Visual Studio, x86 and x64, debug and release builds.

The VS 2017 jobs are very slow, they seem to get stuck on choco install visualstudio2017-workload-vctools for a while, and the whole "install visual studio" step takes about 6 minutes for VS 2017 (2019 and 2022 don't need this extra install step). Do we need to test against VS 2017? I'm guessing we do because that's what the current AppVeyor uses.

I always opted to keep this in for backwards compatibility, but by now we're like 7 years further so imo VS2017 is not really crucial anymore. For the variants we could for example build the dev one for Debug and Release, and the standard one only for Release? Likewise only build the VS2019 in Release.

@dpgeorge
Copy link
Member

dpgeorge commented Feb 5, 2024

OK, I think this is working now. Needed a small tweak to remove the space between the variable name and the variable value.

I would think this is the issue described here yet you fixed this with only one Trim so not sure.

The issue was that pasting text using "micropython=$((msbuild ...))" inserted a space after the =, ie before the expanded expression.

The fix was to use ("micropython=" + $(msbuild ...)).

@dpgeorge
Copy link
Member

dpgeorge commented Feb 5, 2024

For the variants we could for example build the dev one for Debug and Release, and the standard one only for Release? Likewise only build the VS2019 in Release.

I've now excluded VS 2019 in release mode (the standard variant was already only built in release mode).

The set of jobs are now:

jobs

By moving to GitHub actions, all MicroPython CI builds are now on GitHub
actions.  This allows faster parallel builds and saves time by not building
when no relevant files changed.

This reveals a few failing tests, so those are temporarily disabled until
they can be fixed.

Signed-off-by: David Lechner <david@pybricks.com>
Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the windows-to-github-actions branch from 4620347 to ed15b3c Compare February 5, 2024 22:56
@dpgeorge dpgeorge merged commit ed15b3c into micropython:master Feb 5, 2024
@dpgeorge
Copy link
Member

dpgeorge commented Feb 5, 2024

This is now merged. And I've disabled (but not deleted, yet) the AppVeyor GitHub webhook.

Thanks @dlech for putting this together!

@dlech dlech deleted the windows-to-github-actions branch February 5, 2024 23:17
@stinos
Copy link
Contributor

stinos commented Mar 25, 2024

@dlech do you happen to remember the full story for needing https://github.com/micropython/micropython/blob/master/tests/run-tests.py#L586 and https://github.com/micropython/micropython/blob/master/.github/workflows/ports_windows.yml#L141 ?

Sorry I didn't see this when looking at the PR but that "import/import_file.py" test should already have been excluded by https://github.com/micropython/micropython/blob/master/tests/run-tests.py#L701 but somehow the if os.name == "nt": is not True in the mingw builds on Github Actions. Don't really understand how (thing runs on Windows after all, and the whole point of having pacman install python3 is that it can be used as-is for running the test suite without needing an 'external' Python installation), but it's pretty annoying because now for each test we want to skip on Windows we have to add it in 2 locations in run-tests. Well, or figure out how to make the CI build run like a local one on Windows; if this doesn't ring any bells for you I'll look into it.

@dlech
Copy link
Contributor Author

dlech commented Mar 25, 2024

IIRC, the MSYS Python is compiled for a POSIX environment and therefore uses Unix line endings instead of Windows line endings (and also explains why os.name is not "nt"). So this is why we need to use the different version of CPython - to get one with Windows line endings so that the generated .exp files match between CPython and MicroPython. We aren't compiling MicroPython for the MSYS environment but just using MSYS for the cross-compiler to target Windows so MicroPython is using Windows line endings.

I don't really remember why the import error was different and still broken though.

@stinos
Copy link
Contributor

stinos commented Mar 25, 2024

So this is why we need to use the different version of CPython - to get one with Windows line endings so that the generated .exp files match between

Hmm. I really thought run-tests has replace(b"\r\n", b"\n") in strategic places (both to get micropython and expected test output) just to fix any possible issue there.

Anyway the key issue here is that following the windows README for an MSYS2 installation has none of these problems, and its python version does have 'nt' for os.name.

I'll see if I can get RDP running..

tannewt added a commit to tannewt/circuitpython that referenced this pull request Apr 22, 2025
Add working_directory for subsequent code file
tannewt added a commit to tannewt/circuitpython that referenced this pull request Apr 24, 2025
PR micropython#10263 moved it to only before code.py. This breaks the repl (and
likely boot.py.)

Fixes micropython#10289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants