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

Add RFC96 text: Deferred C++ plugin loading #8648

Merged
merged 8 commits into from
Nov 17, 2023
Merged

Conversation

rouault
Copy link
Member

@rouault rouault commented Nov 2, 2023

Partially rendered view here

@rouault rouault force-pushed the rfc96_text branch 3 times, most recently from 3695e47 to 0749abe Compare November 2, 2023 11:58
when there are differences.

GDALDataset::Open(), Create(), CreateCopy() methods are modified to not use
directly the pfnOpen, pfnCreate, pfnCreateCopy callbacks (that would be the ones
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change really necessary? Wouldn't be possible to delegate the loading to the pfn* in the proxy, maybe a generic function of the proxy that will load (once) the real driver and then call the real driver's pfn* ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I acknowledge that this is not super pretty, but it is intended to be GDAL core low-level internal detail. The issue is for example that GDALDriver::Create() currently calls pfnCreate on the proxy driver, but it doesn't know that it is a proxy driver. When the proxy driver has not loaded yet the real driver, the pfnCreate callback is null... Hence this GetCreateCallback() virtual method implemented in the proxy driver that takes care of loading the real driver and setting the pfnCreate callback of the proxy with the value of the pfnCreate of the real driver. The alternative would be that each place that accesses the callbacks checks if the driver object derives from GDALPluginDriverProxy to force it to load the real driver, but I don't find that solution prettier.

Copy link
Collaborator

@elpaso elpaso left a comment

Choose a reason for hiding this comment

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

Looks good to me.
A big improvement.
I'd probably liked even more if the sidecar file solution would be implemented, but I guess that if that would work well with exposing metadata, for identify and subdataset it must necessarily be done in C++ code.

@tbonfort
Copy link
Member

tbonfort commented Nov 7, 2023

LGTM given that the scope is limited to deferred loading of in-tree drivers, but this has limited utility in the scope of rfc85 where we try to keep large/external/proprietary drivers out of tree

I'm wondering if it would be possible to obtain the same outcome with completely out-of-tree drivers, using some kind of code-generation at cmake time:

  • cmake accepts -DADD_DEFERRED_PLUGIN_FOO=/path/to/foodriver/core.cpp with core.cpp containing the well-known functions FOODatasetIdentify, FOODriverSetCommonMetadata and DeclareDeferredFOOPlugin (in the same way that for traditional plugins we expect a gdal_foo.so file to contain a GDALRegister_foo() function)
  • add the /path/to/foodriver/core.cpp to the list of files to be compiled into the main libgdal.so library
  • use code generation to create the DeclareDeferredPlugins() function that will be called by GDALAllRegister():
// this file is automatically generated and will be overwritten, do not edit manually
void DeclareDeferredPlugins() {
  DeclareDeferredFOOPlugin(); //from -DADD_DEFERRED_PLUGIN_FOO
  DeclareDeferredBARPlugin(); //from -DADD_DEFERRED_PLUGIN_BAR
}

@rouault
Copy link
Member Author

rouault commented Nov 7, 2023

I'm wondering if it would be possible to obtain the same outcome with completely out-of-tree drivers

good point. Turns out to be quite straightforward. I've added a new commit in the RFC with that capability and candidate implementation in rouault@7f9c686

@rouault
Copy link
Member Author

rouault commented Nov 12, 2023

Candidate implementation available in #8695

…LATE_FROM to the list of items that must be set on the proxy
@rouault rouault changed the title Add RFC96 text: Deferred in-tree C++ plugin loading Add RFC96 text: Deferred C++ plugin loading Nov 15, 2023
@rouault rouault added this to the 3.9.0 milestone Nov 17, 2023
@rouault rouault merged commit 892ce4f into OSGeo:master Nov 17, 2023
1 of 2 checks passed
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