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 Windows to CI tests #2968

Merged
merged 5 commits into from
Nov 30, 2024
Merged

Add Windows to CI tests #2968

merged 5 commits into from
Nov 30, 2024

Conversation

tdcosta100
Copy link
Collaborator

@tdcosta100 tdcosta100 commented Oct 25, 2024

Until now, except for Node-CI, Windows didn't have tests yet. So now I'm adding them, based on Linux-CI. I created a matrix with 4 renderers (OpenGL, EGL, Vulkan and OSMesa), and 2 rendering modes (Legacy and Drawable). Two combinations are disabled:

  • EGL/Drawable (the tests are throwing exceptions, I don't know what causes it or how to fix. More details)
  • Vulkan/Legacy - since this combination does not exist in Linux, I'm doing the same in Windows

I left some considerations on ignored tests, so others can investigate and fix them in the future. Also, some unit and render tests appear to have specific Windows issues (like font rendering, for example, which will always have some differences, and some number precision issues), so I tweaked these cases, but this is not a definitive solution and suggestions are welcome.

Closes #917

Copy link

github-actions bot commented Oct 26, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [ = ]       0  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2968-compared-to-main.txt

Copy link

github-actions bot commented Oct 26, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [ = ]       0  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2968-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +30% +34.5Mi  +432% +25.8Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2968-compared-to-legacy.txt

Copy link

github-actions bot commented Oct 26, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0036         -0.0030             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2968-compared-to-main.txt

@tdcosta100 tdcosta100 force-pushed the windows-ci branch 3 times, most recently from d909aa5 to 9465a26 Compare October 26, 2024 05:32
test/CMakeLists.txt Outdated Show resolved Hide resolved
@louwers louwers added build Related to build, configuration or CI/CD windows labels Oct 26, 2024
@tdcosta100 tdcosta100 force-pushed the windows-ci branch 3 times, most recently from 3f4a7f8 to b281110 Compare October 26, 2024 17:38
@tdcosta100 tdcosta100 force-pushed the windows-ci branch 2 times, most recently from 11404ee to fc75f09 Compare November 11, 2024 01:04
@louwers louwers added the github_actions Pull requests that update GitHub Actions code label Nov 11, 2024
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Maybe we should disable the metrics for Windows. It's going to be a bit of a pain to have to update them for yet another platform when changing anything. Actually I think ideally we should have some workflow to update the metrics automatically on a PR.

Anyway, that is a concern for later. LGTM!

@tdcosta100
Copy link
Collaborator Author

Maybe we should disable the metrics for Windows. It's going to be a bit of a pain to have to update them for yet another platform when changing anything. Actually I think ideally we should have some workflow to update the metrics automatically on a PR.

Anyway, that is a concern for later. LGTM!

Well, maybe. But redoing metrics it's kind of easy to do. My biggest concern here is testing on Qt platform, so I still need @ntadej to update the Docker image to use CMake 3.24.

@louwers
Copy link
Collaborator

louwers commented Nov 27, 2024

@tdcosta100 I think the plan is to drop support for Qt5. Would that be sufficient?

@tdcosta100
Copy link
Collaborator Author

@tdcosta100 I think the plan is to drop support for Qt5. Would that be sufficient?

I think so, but until then, how can we pass this PR in the tests?

@louwers
Copy link
Collaborator

louwers commented Nov 30, 2024

Either ignore the Qt workflows (they are not required for merging) or remove all matrix blocks in qt-ci that mention Qt 5. https://github.com/maplibre/maplibre-native/blob/main/.github/workflows/qt-ci.yml#L80-L87

Windows workflows seem to be timing out...

@tdcosta100
Copy link
Collaborator Author

I will see what's going wrong and also update it to match the main branch.

@tdcosta100 tdcosta100 force-pushed the windows-ci branch 2 times, most recently from 9c61338 to 44a455e Compare November 30, 2024 05:26
@tdcosta100
Copy link
Collaborator Author

Found the reason. With the release of CMake 3.31, vcpkg stopped working properly in CI. I will PR an update for it.

@tdcosta100
Copy link
Collaborator Author

I fixed the Qt5 configuration in this branch, CMake version is bigger than 3.24, so everything works as indended. Now tests are passing flawlessly (at least for while).

@ntadej
Copy link
Collaborator

ntadej commented Nov 30, 2024

Looks OK to me!

Sorry for silence, I was very busy lately.

@louwers
Copy link
Collaborator

louwers commented Nov 30, 2024

@ntadej No worries, thanks for having a look!

@louwers louwers merged commit e932cae into maplibre:main Nov 30, 2024
49 checks passed
@tdcosta100 tdcosta100 deleted the windows-ci branch November 30, 2024 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to build, configuration or CI/CD github_actions Pull requests that update GitHub Actions code windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run Windows build on CI with MBGL_EGL = ON
3 participants