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

GDExtension: Copy DLL to a temp file before opening #80188

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

vnen
Copy link
Member

@vnen vnen commented Aug 2, 2023

This is done only in the editor and only on Windows, to avoid a file lock that prevents the original library being updated (e.g. by a compiler).

When the game runs it will load the original DLL and pick up any changes, only the editor will stay with the copy (until it is restarted and create a new copy).

The copy is done in place by prepending a ~ to the original file name, so dependencies that are loaded with a relative file path still work. When the library is unloaded the copy file is deleted.

Closes godotengine/godot-cpp#955

Note: This still does not enable actual reloading for GDExtension, but it is a step towards it.

@vnen vnen added this to the 4.2 milestone Aug 2, 2023
@vnen vnen requested review from a team as code owners August 2, 2023 20:39
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 great!

I finally got a new Windows development environment going so I could test this. Without this PR, I got "access denied" when trying to re-compile my GDExtension with the editor open. But with this PR, I was able to re-compile just fine, and the temporary copy got cleaned up when the editor closed.

The code looks good to me too.

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.

Looks fairly straightforward.

Didn't test, CC @bruvzg if you're interested in checking it, but should be mergeable as is.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

I would probably make the temp file hidden (but I do not think we have an API for this).

core/extension/gdextension.cpp Outdated Show resolved Hide resolved
@vnen
Copy link
Member Author

vnen commented Aug 7, 2023

I would probably make the temp file hidden (but I do not think we have an API for this).

Yeah, I couldn't find a way to make it hidden.

@vnen
Copy link
Member Author

vnen commented Aug 7, 2023

Updated to remove the define.

@akien-mga
Copy link
Member

Needs rebase to fix a temporary extension compat error.

@Bromeon
Copy link
Contributor

Bromeon commented Aug 7, 2023

Very very nice, fantastic job! 💖

I manually rebased this onto f2acfb1 and tried it with godot-rust.
Only tested simple examples so far, but it seems to work as expected! 👍

@akien-mga
Copy link
Member

You could now update this to use the read only attribute added in #80404.

This is done only in the editor and only on Windows, to avoid a file
lock that prevents the original library being updated (e.g. by a
compiler).

When the game runs it will load the original DLL and pick up any
changes, only the editor will stay with the copy (until it is restarted
and create a new copy).

The copy is done in place by prepending a `~` to the original file name,
so dependencies that are loaded with a relative file path still work.
When the library is unloaded the copy file is deleted. The copy is also
marked as hidden to not show up in explorer.
@vnen
Copy link
Member Author

vnen commented Aug 11, 2023

Updated to set the temp file as hidden. I didn't set the read attribute because I think it's not relevant (the file can't be written anyway because of the open lock). Given the name it's also set as an "protected OS file" so it does not show unless you have the option on to show these types of files, which is an extra bonus.

@dsnopek
Copy link
Contributor

dsnopek commented Aug 11, 2023

Discussed at the GDExtension, and we think this looks awesome!

@dsnopek
Copy link
Contributor

dsnopek commented Aug 11, 2023

I retested this with the latest changes, and I noticed a minor issue: if the .dll exists but fails to load for any reason, the copy won't get cleaned up when the editor closes. Probably this should delete the copy right away if there's an error opening the library.

Other than, still seems to be working great!

@akien-mga
Copy link
Member

I retested this with the latest changes, and I noticed a minor issue: if the .dll exists but fails to load for any reason, the copy won't get cleaned up when the editor closes. Probably this should delete the copy right away if there's an error opening the library.

It probably just requires adding some cleanup code on each possible early return/fail condition in the loading function.
Can be done as a follow-up, let's merge the current version for wider testing.

@dsnopek
Copy link
Contributor

dsnopek commented Aug 16, 2023

Can be done as a follow-up, let's merge the current version for wider testing.

Sounds good! I can make the follow up :-)

@akien-mga akien-mga merged commit 1e3b1a7 into godotengine:master Aug 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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