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

SurfaceUpgradeTool breaks using --headless --editor --quit in CI/CD to generate .godot/extension_list.cfg (and probably other things too) #84460

Closed
dsnopek opened this issue Nov 4, 2023 · 6 comments

Comments

@dsnopek
Copy link
Contributor

dsnopek commented Nov 4, 2023

Godot version

v4.2.beta.custom_build [be386e1]

System information

Ubuntu 22.04

Issue description

Godot needs some files in .godot/ in order to run a game, so if you have a fresh checkout of your project's code, you need to run it in the editor at least once.

A common solution to this on CI/CD, is to run godot --headless --editor --quit and then run your tests or export your game. (This is used in the godot-cpp CI in order to generate the .godot/extension_list.cfg which is needed to load a project's GDExtensions, which is how I discovered this issue.)

After PR #84200 this no longer works

That PR delays when EditorFileSystem::scan() runs (which is what will generate .godot/extension_list.cfg) and so what appears to happen is that the editor is quitting before it has a chance to scan. The --quit causes Godot to quit after 1 frame, but now the EditorFileSystem::scan() doesn't happen until a later frame, so we're quitting too early.

I suspect that there may be other files in the .godot/ directory that should be generated (for example global_script_class_cache.cfg), but aren't now after PR #84200.

Steps to reproduce

  1. Checkout godot-cpp
  2. Run godot --path test/project --headless --editor --quit
  3. List the test/project/.godot/ directory, and notice that extension_list.cfg is not present

Minimal reproduction project

Use godot-cpp

@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 4, 2023

cc @clayjohn

@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 4, 2023

cc @YuriSizov @akien-mga

@akien-mga
Copy link
Member

akien-mga commented Nov 4, 2023

Yeah that's a problem. We could maybe add a hack that sidesteps this delay (and the upgrade tool?) when running headless?

The proper fix should be to have a proper --import flag that properly ensures the scan happens and finishes before quitting the editor. But that's not a short term option for 4.2.

A user workaround should be to use --quit-after 5 (or more) instead of --quit, but we still need to fix the behavior with --quit as it's very widely used.

@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 4, 2023

It looks like adding --quit-after 100 works in my testing - see godotengine/godot-cpp#1299

Locally, it wouldn't work with --quit-after 10 but did with --quit-after 12, so just to be safe I put --quit-after 100 in the CI.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 6, 2023

I think this would be acceptable for now, as there are already issues with the described trick, which are only solvable with --quit-after, as far as I know. If we really really need an engine-side hack, then as @akien-mga suggests we can check for the headless mode and do the scan at the previous position in the loading flow.

I'm going to look into streamlining the loading steps of the editor in 4.3. Hopefully having clear loading steps will make it easier to implement the --import flag.


Edit:

Although I'm not sure how it works right now (before our PR). It should quit immediately after one iteration with the --quit flag, and scanning should take multiple frames. I don't see any guards in there that would prevent quitting.

... But I have an idea. I guess whenever we call Main::iteration() not from the platform code, we never check the exit code? So we may be accidentally iterating multiple times before the platform code has any chance to check for exit? Is this intentional?..

And we call Main::iteration() from the process dialog that pops up whenever we do the reimport. Which I guess still works in the headless mode.

@YuriSizov
Copy link
Contributor

Addressed with #84570.

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

No branches or pull requests

3 participants