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

Export: Don't export global class list if none is used #94540

Closed

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jul 19, 2024

Draft for now as I need to confirm when this happens exactly.

I noticed this error when exporting the MRP from #94537 to the web (one click deploy), which uses gdextension.

I don't see the same in a MRP with nothing at all, so more investigation is needed.

Edit: Actually I do, here's a MRP:
testexportsimple.zip

@akien-mga akien-mga added this to the 4.3 milestone Jul 19, 2024
@akien-mga akien-mga marked this pull request as ready for review July 31, 2024 12:55
@akien-mga
Copy link
Member Author

akien-mga commented Jul 31, 2024

So this actually seems to be a regression from #92303. CC @Hilderin

Before that PR, the config file would always be written, even if empty:

void EditorFileSystem::_update_pending_script_classes() {
	if (!update_script_paths.is_empty()) {
		_update_script_classes();
	} else {
		// In case the class cache file was removed somehow, regenerate it.
		if (!FileAccess::exists(ScriptServer::get_global_class_cache_file_path())) {
			ScriptServer::save_global_classes();
		}
	}
}

But now it's not written if empty.

Unless that's necessary for the fixes in #92303, I would suggest that we restore the previous behavior to always write the config file. Editor code seemed to assume it (which we can fix like here), but user plugins may also do their own attempts at parsing this file and might start failing if they weren't properly checking for its existence.

BTW, this removal also makes ScriptServer::get_global_class_cache_file_path() unused, so it could be removed too. Even if we re-add that check, it could use ProjectSettings::get_singleton()->get_global_class_list_path() directly, it's not clear to me why reduz added this extra method for it in 79897dd.

@Hilderin
Copy link
Contributor

Indeed, it's a regression from #92303. Because of the early return in EditorFileSystem::_update_script_classes which was added last minute, the config file is never rewritten when no global class are used.

I'll create a PR later to add that in the early return:

if (!FileAccess::exists(ProjectSettings::get_singleton()->get_global_class_list_path())) {
    ScriptServer::save_global_classes();
}

And I'll remove ScriptServer::get_global_class_cache_file_path().

Doing that, this PR maybe is not that useful?

@akien-mga
Copy link
Member Author

Yeah that sounds good.

I'll close this PR as the error was helpful to find a case where the file is missing, so we can keep the guarantee that this file should always be generated on first import.

@akien-mga akien-mga closed this Jul 31, 2024
@akien-mga akien-mga removed this from the 4.3 milestone Jul 31, 2024
@akien-mga akien-mga deleted the export-no-global-classes branch July 31, 2024 22:11
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