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

Test Godot with Vulkan in CI #47414

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Mar 27, 2021

Similar to #44767 but with this will allow to test Godot with Vulkan renderer.

This should allow to catch some types of bugs/crashes etc. in CI without needing to run project locally on machine.
Due using Address Sanitizer it will show exactly place where something went wrong.

Used project is https://github.com/qarmin/RegressionTestProject/tree/4.0 which is a little changed version of already used in CI 3.x branch project. It contains only a few independent scenes with some scripts, so it should be really easy to create minimal project and track bugs.

I deleted Mono from this build because it crashes Godot when opening editor(not found assembly) and already exists in CI mono build(template)(PR #47600 aims to fix it)

This build needs debug_symbols option in scons, so I also added it.

It takes a little more time than similar CI in 3.x branch, but I still think that this will save a lot of users time, that they would have to spend debugging the project/engine.

One check in engine about tesselation is disabled because caused problems with SwiftShader.

It use prebuild SwiftShader library which are hosted on Github Releases(in future it should be hosted in official Godot repository).
Compilation instructions are available in this issue #38428

@qarmin qarmin requested a review from a team March 27, 2021 17:38
@qarmin qarmin force-pushed the vulkan_swiftshader_ci branch 2 times, most recently from defd697 to f015166 Compare March 27, 2021 19:04
@qarmin qarmin marked this pull request as ready for review March 27, 2021 19:04
@qarmin qarmin added this to the 4.0 milestone Mar 27, 2021
@qarmin
Copy link
Contributor Author

qarmin commented Mar 29, 2021

Looks that without problems Godot is able to run test project in 1h long CI - https://github.com/qarmin/RegressionTestProject/runs/2220082717?check_suite_focus=true

The only problem which I see is that Godot randomly freeze when showing some elements(even 30s freeze sometimes).
This happens also on PC, but it shouldn't interfere too much with the execution of CI.

EDIT: Looks that computing/loading scene with some physics 3d object cause this freeze.
In Editor additionally looks that debugger cause more freezing.

@qarmin qarmin force-pushed the vulkan_swiftshader_ci branch 3 times, most recently from f11f0cc to d8d2741 Compare April 3, 2021 16:55
@qarmin
Copy link
Contributor Author

qarmin commented Apr 15, 2021

Bevy already setup CI with help of this github workflow - bevyengine/bevy@d868d07

@@ -76,9 +76,9 @@ jobs:
path: bin/*
retention-days: 14

linux-editor-sanitizers-mono:
Copy link
Member

Choose a reason for hiding this comment

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

Why not meld the changes from #47600 directly in this PR? With both PRs we don't need to disable Mono in the first place, do we?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see it's not affecting the same build, makes sense then.

@@ -126,17 +126,47 @@ jobs:
scons --version

# We should always be explicit with our flags usage here since it's gonna be sure to always set those flags
# [Workaround] SwiftShader have problems with tesselation, so we comment this for now
Copy link
Member

Choose a reason for hiding this comment

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

Is there an upstream bug report about this, so we can track resolution?

Copy link
Member

Choose a reason for hiding this comment

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

[Workaround] SwiftShader have problems with tesselation, so we comment this for now

Tessellation is not implemented at all, and probably software implementation is not feasible for performance reasons.

Is there an upstream bug report about this, so we can track resolution?

There's feature proposal in the SwiftShader bug tracker Implement tesselation support [147805271], but it's for ANGLE/OpenGL ES, and there's no activity.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably change Godot code to handle missing tessellation gracefully then if we want to use SwiftShader (and let others use it).
This sed workaround is not really sustainable in the long run.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Left a nitpick but otherwise seems good to me, great job 👍

Would be worth a rebase to ensure that it's still passing on current master.

# Download, unzip and setup SwiftShader library
- name: Download SwiftShader
run: |
wget https://github.com/qarmin/gtk_library_store/releases/download/3.24.0/swiftshader.zip
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to document what version of SwiftShader this is / how it was built. I just spent 5 minutes going through the upstream repo trying to find any reference to a 3.24.0 release...

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 added commit number to comment.
Compilation instructions are in #38428 (comment) and #38428 (comment)

SwiftShader is currently developing quite slowly, so I think an update will only be necessary in no less than 3 months.

@akien-mga akien-mga merged commit 3890870 into godotengine:master Apr 15, 2021
@akien-mga
Copy link
Member

Thanks!

@qarmin qarmin deleted the vulkan_swiftshader_ci branch April 16, 2021 04:27
@madmiraal
Copy link
Contributor

So how do we address CI failures from compat breaking changes? For example, #47789 now fails the CI checks, because the test project is using the old name:
https://github.com/godotengine/godot/pull/47789/checks?check_run_id=2369223941

@qarmin
Copy link
Contributor Author

qarmin commented Apr 17, 2021

I was thinking about it, and one of solutions is to disable step with running project, merge breaking changes, re-enable checking project when test project will be updated, but this require 2 additional commits and non working test project checking for even few days.

I think that the best idea is just put to CI simple sed command - to not break CI of users which doesn't rebase immediately its PR(this command should be deleted after few days or weeks)
For renaming OS to Platform should be enough to put this command

find test_project -name "*.gd" -exec sed -i "s/OS/Platform/g" '{}' \;

at the beginning of this step

# Run test project
- name: Run project
run: |
VK_ICD_FILENAMES=$(pwd)/vk_swiftshader_icd.json DRI_PRIME=0 xvfb-run bin/godot.linuxbsd.tools.64s 40 --audio-driver Dummy --path test_project 2>&1 | tee sanitizers_log.txt || true
misc/scripts/check_ci_log.py sanitizers_log.txt

I only found 3 changes which probably could brake CI

@madmiraal
Copy link
Contributor

All changes need to be atomic, and changes should definitely not be modifying CI checks (unless they are specifically updating the CI check). I don't even want to begin expressing my horror at the suggestion that those changes be made on the fly with sed.

On a more general note, CI checks that are be dependent on third-party projects are at the mercy of those projects. We've had CI checks suddenly start failing in the past, because of third-party updates (see for example #39168). I think it's a bad idea to make Godot's CI checks dependent on a separate test project, never mind a third-party test project. We have created a whole bunch of unit tests (https://github.com/godotengine/godot/tree/master/tests) that should be used for regression testing.

If a PR fails a CI check, either the PR is flawed or the CI check is flawed. In this case it's the CI check that is flawed. This PR prevents compat breaking changes from being made, and makes it dependent on an arbitrary third-party project. Therefore, I think this change should be reverted.

@qarmin
Copy link
Contributor Author

qarmin commented Apr 18, 2021

Of course, the test project stored in an external repository is not the best idea, but for now it is better than nothing and I hope that in time a new official project will be created based on it.
The same project has been working in the 3.2/3.x branch for over half a year and has already prevented several bugs such as #43162, #46392, #40313 (comment)
Additionally, in case of any problems, throwing it out of CI is just deleting a few lines.

Godot has unit tests, but they check only a very small group of code (I think that probably something around 1%).

Godot 4.0 is still changing very dynamically, so various regressions are possible at any time, and it should use as many tests as possible.
So far I've manually tested only a part of the PRs with this project, but I still found quite significant errors - #47727, #47569, #45852, #45672, #43725, #43323, and CI will allow to find them even faster in every PR.

I understand that when renaming functions or doing other breaking changes, adding a sed command to CI is not very elegant, but in my opinion this is a very small price to pay for fully automated Godot testing, which can save a lot of users and developers time.

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

Successfully merging this pull request may close these issues.

None yet

4 participants