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

Fix failing CI, due invalid importing of resources #43201

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Oct 30, 2020

This should fix errors about leaks(due not importing all resources properly) like in this run - https://github.com/godotengine/godot/pull/43049/checks?check_run_id=1329394149

Godot for now doesn't have any method to safely import all resources.
The best option would be to create a command line argument to import all assets - godotengine/godot-proposals#1362

Importing is done 3 times, because I want to be sure that everything should works fine.

@qarmin qarmin added this to the 3.2 milestone Oct 30, 2020
@akien-mga
Copy link
Member

Why did CI start failing now? It uses to work fine for weeks.

Having two runs of 25 s each before the last one feels pretty hacky and might add extra time to an already fairly long build... I'll merge as is to fix CI but I'd like to see this improved further.

@akien-mga akien-mga merged commit e6d0770 into godotengine:3.2 Oct 30, 2020
@akien-mga
Copy link
Member

Thanks!

@qarmin
Copy link
Contributor Author

qarmin commented Oct 30, 2020

Before RegressionTestProject doesn't have any images/resources which needs to import(even icon.png was missing), so it will work even without opening editor.

But yesterday I pushed new commit with 2 new images - 32x32 icon.png(~7KB) and 512x512 icon.png(~60KB) and since this time, CI started to fail randomly.

Godot with address sanitizer opens editor a little longer than normal and 25s should be enough to calmly import all assets, but as CI shows, this doesn't work.

This looks like a bug in Godot, but I don't know for now what is cause of this behavior and even how to debug it.

@akien-mga
Copy link
Member

akien-mga commented Oct 31, 2020

Some PRs are failing due to these new changes: https://github.com/godotengine/godot/pull/43126/checks?check_run_id=1336423811

I think it would be better to pin a specific commit of RegressionTestProject so that CI doesn't break on updates, and test updates you want to make by changing the pinned commit in a PR. Having unrelated PRs fail their CI checks due to third-party repo changes is not a good workflow.

In particular, I think that any PR made before yesterday will now fail CI whenever the author pushes an update without first doing git pull --rebase of the upstream 3.2 branch to get these changes.

@qarmin
Copy link
Contributor Author

qarmin commented Oct 31, 2020

Now it shouldn't break on any project updates, because project CI is almost same as Godot CI(have 2 not 3 editor opening steps and run project for 60 second instead 20, so it should easier find bugs).

https://github.com/qarmin/RegressionTestProject/blob/3.2/.github/workflows/linux_builds.yml

For this reason I think that pinning a specific version of the project is not necessary (until of course there are some bugs in the latest version, but CI should prevent this)

@akien-mga
Copy link
Member

Sounds good, if you test changes with the same CI job on RegressionTestProject before merging, that's fine with me to keep tracking HEAD indeed.

@akien-mga akien-mga modified the milestones: 3.2, 3.3 Apr 21, 2021
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.

2 participants