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

Codechange: refactor the Windows-only DllLoader in a cross-platform LibraryLoader #11751

Merged
merged 1 commit into from Jan 10, 2024

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 10, 2024

Motivation / Problem

For another PR (#11628) I had need to dynamically load a library on any OS. We already had some code to do something similar for Windows only (in DllLoader). So this PR extends its capabilities and makes it cross-platform.

Description

DllLoader was very scoped around Windows (both in name and implementation). This PR recreates the API of this loader, making sure it can also be implemented for Linux and MacOS.

When-ever an instance of LibraryLoader gets out of scope, everything is cleaned up nicely. This means that any function pointer you retrieve from it only remains valid for as long as the object lives. It -might- live longer, but that is OS-dependent, and kinda depends if anything else still has the library loaded or not.

During implementation I found out that some headers were missing from other files, now <windows.h> wasn't included anymore in os/windows/win32.h.

LibraryLoader has functions to see if loading succeeded and/or what the error was (if any). This helps in producing nice error-messages to the user, instead of internal error codes.

PS: yes, the MacOS implementation is in there: it is the same as the Linux implementation, and both OSes share the implementation from os/unix/.., as is already the case with some other pieces of code in there.
PPS: no, Emscripten is not (and will not) be supported, as there is no concept of "library" in that sense. We could, in theory, make them WASM plugins, but that requires other infrastructure to load in.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

rubidium42
rubidium42 previously approved these changes Jan 10, 2024
src/library_loader.h Outdated Show resolved Hide resolved
src/os/windows/library_loader_win.cpp Outdated Show resolved Hide resolved
src/library_loader.h Show resolved Hide resolved
src/os/unix/library_loader_unix.cpp Outdated Show resolved Hide resolved
src/library_loader.h Outdated Show resolved Hide resolved
src/os/unix/unix.cpp Outdated Show resolved Hide resolved
src/os/windows/win32.h Outdated Show resolved Hide resolved
@rubidium42 rubidium42 dismissed their stale review January 10, 2024 20:46

Think before you do...

@TrueBrain TrueBrain enabled auto-merge (squash) January 10, 2024 21:27
@TrueBrain TrueBrain merged commit d3ee045 into OpenTTD:master Jan 10, 2024
20 checks passed
@TrueBrain TrueBrain deleted the load-library branch January 10, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants