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 extensions loading/initializing even when entry point fails #82861

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Oct 5, 2023

This fixes an apparent bug with the loading of extensions, where even if false is returned from the extension's entry point (here) the extension will still be considered successfully loaded by GDExtensionManager (here) due to the p_extension parameter pointing to a valid instance even after the entry point has reported a failure. This also results in it calling the extension module initalizer function even after its entry point has failed, which doesn't seem right.

This also has the unfortunate side effect of preventing any actual error messages from being emitted to the editor log as a result of the failure in the extension entry point, since EditorLog is initialized as part of Main::start(), rather than Main::setup() where all the extension initialization happens. Currently you're able to see editor logs/errors for things like compatibility_minimum due to the fact that any loading of extensions gets retried as part of EditorFileSystem::_scan_extensions, but this will only ever actually be retried if the loading failed in the first place, which it doesn't without the change in this PR.

CC: @dsnopek

@mihe mihe requested a review from a team as a code owner October 5, 2023 16:07
@mihe
Copy link
Contributor Author

mihe commented Oct 5, 2023

@dsnopek I also noticed that the library itself doesn't get closed when the entry point fails, leading to the library still technically being loaded by the process and thus locked on Windows, preventing the cleanup here from happening properly. I can throw that into this PR if you want.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 5, 2023

Thanks!

I also noticed that the library itself doesn't get closed when the entry point fails, leading to the library still technically being loaded by the process and thus locked on Windows

Eep, yes, it'd be great if you could add that to this PR as well!

@dsnopek dsnopek added this to the 4.2 milestone Oct 5, 2023
@mihe mihe force-pushed the gdext-entry-false branch from f1d7283 to 5c6353a Compare October 5, 2023 16:25
@mihe
Copy link
Contributor Author

mihe commented Oct 5, 2023

Eep, yes, it'd be great if you could add that to this PR as well!

Done.

Also, as an aside, I think the UX for when an extension fails to load should probably be looked over. It currently feels quite aggressive in the way it shows you not only a modal popup saying what extension failed to load, but also multiple toast messages. Not only that, it ends up doing this every time it scans for extensions, which seems to be quite frequently, which leads to the modal popup becoming quite annoying. I feel like ditching the modal popup would go a long way, since it's somewhat redundant with all the other errors being emitted, although I'm not entirely sure where it's originating from.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me!

Also, as an aside, I think the UX for when an extension fails to load should probably be looked over. It currently feels quite aggressive in the way it shows you not only a modal popup saying what extension failed to load, but also multiple toast messages.

I agree, the "toast messages" are probably enough. Something to address in another PR :-)

@akien-mga akien-mga merged commit 7c56631 into godotengine:master Oct 5, 2023
@akien-mga
Copy link
Member

Thanks!

@mihe mihe deleted the gdext-entry-false branch October 5, 2023 21:22
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.

3 participants