use a singleton pattern to register the gdal driver once in one place #4628
Conversation
| @@ -169,7 +186,7 @@ std::string serializeGeoTIFF(Api& request, const std::shared_ptr<const GriddedDa | |||
| char** geotiff_options = NULL; | |||
| geotiff_options = CSLSetNameValue(geotiff_options, "COMPRESS", "PACKBITS"); | |||
|
|
|||
| // TODO: check if there's an easy way to only instantiate the driver once | |||
| gdal_singleton_t::get(); | |||
There was a problem hiding this comment.
this first time this is called (per process) the constructor above is called and the driver registration happens. subsequent calls are no-ops
| @@ -191,12 +191,15 @@ else() | |||
| message(FATAL_ERROR "Required target protobuf::libprotobuf-lite or protobuf::libprotobuf is not defined") | |||
| endif() | |||
|
|
|||
| # gdal | |||
| set(gdal_target "") | |||
| # unless you said you didnt want gdal we try to turn it on, if we cant we tell you | |||
There was a problem hiding this comment.
i thought it would be better to default turning it ON if we can and if we couldnt we simply report that was the case. if you tell it you definitely dont want it by setting it to OFF then we dont even try
There was a problem hiding this comment.
Hm but doesn’t OFF still want to pull in GDAL::GDAL in tyr’s cmakelists? That’s what that variable was for, though you could handle that with a cmake expression in tyr as well, that’s probably more explicit than the previous var
There was a problem hiding this comment.
yep will do that real quick
There was a problem hiding this comment.
Great, thanks. Just one question about GDAL=OFF @kevinkreiser . Could even do one CI build without, but no need to either IMO
ha! yeah i grepped the |
|
Oh jeez I’m really sorry, lousy review: I forgot about the docs in docs/docs/building.md, they still say default is OFF. Anyways no worries if you don’t feel like it |
| @@ -355,7 +355,6 @@ void check_raster_edges(size_t x, size_t y, uint16_t* data) { | |||
| } | |||
|
|
|||
| TEST(Isochrones, test_geotiff_output_distance) { | |||
| GDALRegister_GTiff(); | |||
There was a problem hiding this comment.
i dont really like removing these because the tests themselves use gdal directly to load the response from the library. now in this case technically we can remove them because hidden in the source of the serializer we are doing this initialization but the tests (and just in general any calling code) shouldnt know/assume that its preconditions are met by something it calls into. the only way to get around this would be to expose gdal initialization as a public api in the library and have the tests call that but i hate that idea even worse.
There was a problem hiding this comment.
Ah I didn’t check that.. I kinda assumed those driver inits were there to make the test work (it wouldn’t have without). That’s true.. anyways, the alternative to test this bit is indeed worse.
There was a problem hiding this comment.
the other idea is to leave them there though what i would do is in the suite initialization call it once
There was a problem hiding this comment.
Both is fine with me tbh, I get your point, but also think it’ll be fine probably forever ®
fixes #4623 (hopefully)