Set OGC GeoPackage as default export format

[jachym]

Till now, ESRI Shapefile was used as default export format for v.out.ogr.

This patch changes it to OGC GeoPackage.

I'm attaching the patch here to this ticket for further discussion, but can commit by myself any time

[neteler]

How to deal with this packaging case?

On Wed, Oct 18, 2017 at 2:36 PM, Even Rouault wrote:

the SQLite dependency in GDAL builds is optional, so in theory you could have a GDAL build without SQLite/GPKG support. But this is really unlikely in practice as no sane packager would do that. Perhaps checking if the GPKG driver is there would be safer, and if not fallback to good old shapefile.

... hence add a #ifdef condition to the parser default?

[mmetz]

Replying to neteler:

How to deal with this packaging case?

On Wed, Oct 18, 2017 at 2:36 PM, Even Rouault wrote:

the SQLite dependency in GDAL builds is optional, so in theory you could have a GDAL build without SQLite/GPKG support. But this is really unlikely in practice as no sane packager would do that. Perhaps checking if the GPKG driver is there would be safer, and if not fallback to good old shapefile.

... hence add a #ifdef condition to the parser default?

You can add drivers to and remove drivers from your current GDAL version without recompiling GRASS. All GDAL driver checks are done in GRASS on run-time, but #ifdef conditions are evaluated only on compile time. That means, at run-time, every time v.out.ogr is invoked, you would need to check if the "GPKG" driver is available and set the default answer for the format option of v.out.ogr accordingly.

[jachym]

Documentation updated too

[jachym]

Thec checking is done in vector/v.out.ogr/create.c, line 18:

    /* start driver */
    hDriver = OGRGetDriverByName(pszDriverName);
    if (hDriver == NULL) {
	G_fatal_error(_("OGR driver <%s> not available"), pszDriverName);
    }

And this is IMHO ok so.

the html documentation updated too btw.

[jachym]

Any objections to svn commit ?

[mlennert]

Replying to jachym:

Thec checking is done in vector/v.out.ogr/create.c, line 18:

    /* start driver */
    hDriver = OGRGetDriverByName(pszDriverName);
    if (hDriver == NULL) {
	G_fatal_error(_("OGR driver <%s> not available"), pszDriverName);
    }

This would mean that if the user does not have SQLITE support in their GDAL instance, they would get an error message when trying to export using v.out.ogr and its default settings ?

[mmetz]

Replying to mlennert:

Replying to jachym:

Thec checking is done in vector/v.out.ogr/create.c, line 18:

    /* start driver */
    hDriver = OGRGetDriverByName(pszDriverName);
    if (hDriver == NULL) {
	G_fatal_error(_("OGR driver <%s> not available"), pszDriverName);
    }

This would mean that if the user does not have SQLITE support in their GDAL instance, they would get an error message when trying to export using v.out.ogr and its default settings ?

It would be safer to use GPKP as default only if it is available, otherwise default should fall back to ESRI Shapefile.

[jachym]

I'm attaching new version of the patch, which checks the availability of GPKG format on run time

    tempDriver = OGRGetDriverByName("GPKG");
    if (tempDriver == NULL) {
            options->format->answer = "GPKG";
    } else {
            options->format->answer = "ESRI Shapefile";
    }

But I'm not quite convinced, this leads to consistent behavior. I would prefer clear and consistent solution - GPKG is *the* output format, make sure, you have support for it or use different format explicitly (using the format option).

[jachym]

Make sure, GPKG is available

[rouault]

@jachym Your test logic is inverted ;-)

[jachym]

shame on me :-(

Fixed / attached

[neteler]

Patch updated with wording fixes in HTML file

[neteler]

I have fixed the parameter order and a missing "_" in args.c and uploaded under the same "shp-geopackage.4.patch​" name (please use that to continue).

But it does not compile for me:

args.c: In function ‘parse_args’:
args.c:36:5: error: ‘tempDriver’ undeclared (first use in this function); did you mean ‘dbDriver’?
     tempDriver = OGRGetDriverByName("GPKG");
     ^~~~~~~~~~
     dbDriver
args.c:36:5: note: each undeclared identifier is reported only once for each function it appears in

[neteler]

 args.c: 
      tempDriver = OGRGetDriverByName("GPKG");

I found a hint:

​http://www.gdal.org/ogr__api_8h.html#ae814db7e2212b9bbb0fd8c361bee11fe

"Fetch the indicated driver.

NOTE: Starting with GDAL 2.0, it is NOT safe to cast the returned handle to OGRSFDriver*. If a C++ object is needed, the handle should be cast to GDALDriver*.

Deprecated: Use GDALGetDriverByName() in GDAL 2.0 "

[jachym]

@neteler: you are right - we can all see my C skills be going away and never return back.

Something like

 OGRSFDriverH tempDriver;
 char *gpkgname = "GPKG";
 tempDriver = OGRGetDriverByName(gpkgname);

should work - but does not work for me, can you help?

[neteler]

Updated patch

[neteler]

Now it compiles (patch updated) for me but I have no GeoPackage on my current machine (so, "ESRI_Shapefile" is shown).

TODO:

• decide if GDALGetDriverByName() should be used (depends on GDAL version?)
• test on system with GeoPackage present

[rouault]

GDALGetDriverByName() can only be used to detect vector drivers since GDAL 2.0. Previously you had to use OGRGetDriverByName() (OGRGetDriverByName() can still be used in GDAL 2.x)

@neteler Are you using GDAL < 1.11, or perhaps a GDAL build without sqlite3 ?

[rouault]

Oh wait, the reason might be much more mundain (deduced from code review, no actual testing). Apparently OGRRegisterAll() is called by OGR_list_write_drivers(), which is called after this new code... So no driver is available when it is called. Fix: call OGRRegisterAll() much earlier

[neteler]

Replying to rouault:

Oh wait, the reason might be much more mundain (deduced from code review, no actual testing). Apparently OGRRegisterAll() is called by OGR_list_write_drivers(), which is called after this new code... So no driver is available when it is called. Fix: call OGRRegisterAll() much earlier

Bingo! I moved it up, patch updated. Now "GPKG" shows up as a default (I'm on Fedora with GDAL 2.x) :-)

[neteler]

Updated patch

[mmetz]

simplified patch

[mmetz]

Replying to neteler:

Replying to rouault:

Oh wait, the reason might be much more mundain (deduced from code review, no actual testing). Apparently OGRRegisterAll() is called by OGR_list_write_drivers(), which is called after this new code... So no driver is available when it is called. Fix: call OGRRegisterAll() much earlier

Bingo! I moved it up, patch updated. Now "GPKG" shows up as a default (I'm on Fedora with GDAL 2.x) :-)

I have added a somewhat simplified patch that also accounts for GDAL 2.x in list.c.

[jachym]

@neteler: the code is all yours, please commit as soon as you are ok with that :-D

Big thanks for help!

[neteler]

In 71597:

v.out.ogr: Set OGC GeoPackage as default export format (see #3428); contributed by jachym and mmetz

[neteler]

Replying to mmetz:

I have added a somewhat simplified patch that also accounts for GDAL 2.x in list.c.

Thanks, committed as r71597 incl. some further tweaks in the HTML manual.

Backport or not?

[martinl]

Replying to neteler:

Backport or not?

-1 Default vector format for export should not change between point versions of 7.2.

[martinl]

​https://trac.osgeo.org/grass/wiki/Grass7/NewFeatures74?action=diff&version=24

[wenzeslaus]

References:

Switch from Shapefile ​http://switchfromshapefile.org

[GRASS-dev] prefer GeoPackage over Shapefile ​https://lists.osgeo.org/pipermail/grass-dev/2017-October/086299.html

QGIS: Put GeoPackage at first place in the menu ​https://github.com/qgis/QGIS/pull/5385

[neteler]

Replying to martinl:

Replying to neteler:

Backport or not?

-1 Default vector format for export should not change between point versions of 7.2.

Yes, same opinion here. So closing as done.

[sbl]

In tha manual, ESRI_Shapefile is still mentioned as default: https://grass.osgeo.org/grass75/manuals/v.out.ogr.html (probably because the build server does not support GPKG?)

Might be worth noting that GeoPackage requires .gpkg file ending to be GDAL readable (thought that is probably a GDAL version issue...).

[neteler]

Replying to sbl:

In tha manual, ESRI_Shapefile is still mentioned as default: https://grass.osgeo.org/grass75/manuals/v.out.ogr.html (probably because the build server does not support GPKG?)

That's right. It is a meanwhile old Debian server.

Might be worth noting that GeoPackage requires .gpkg file ending to be GDAL readable (thought that is probably a GDAL version issue...).

Should that go into the manual?

[sbl]

Replying to neteler:

Might be worth noting that GeoPackage requires .gpkg file ending to be GDAL readable (thought that is probably a GDAL version issue...).

Should that go into the manual?

Just double checked with newer GDAL version (2.2).That can open GeoPackackages also with non standard endings. So, seems to be a GDAL 2.0 issue: ​https://trac.osgeo.org/gdal/ticket/6396

However, - according to the GDAL ticket above - newer GDAL versions should issue a warning if no standard endings are used...