Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#3428 closed task (fixed)

Set OGC GeoPackage as default export format

Reported by: jachym Owned by: grass-dev@…
Priority: normal Milestone: 7.4.0
Component: Vector Version: svn-trunk
Keywords: v.out.ogr Cc:
CPU: Unspecified Platform: Unspecified

Description

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

Attachments (7)

shp-geopackage.patch (1.1 KB ) - added by jachym 6 years ago.
shp-geopackage.2.patch (4.3 KB ) - added by jachym 6 years ago.
Documentation updated too
shp-geopackage.3.patch (4.5 KB ) - added by jachym 6 years ago.
Make sure, GPKG is available
shp-geopackage.4.patch (4.5 KB ) - added by neteler 6 years ago.
Patch updated with wording fixes in HTML file
shp-geopackage.4.2.patch (4.8 KB ) - added by neteler 6 years ago.
Updated patch
shp-geopackage.4.3.patch (5.2 KB ) - added by neteler 6 years ago.
Updated patch
shp-geopackage.4.4.patch (6.8 KB ) - added by mmetz 6 years ago.
simplified patch

Download all attachments as: .zip

Change History (35)

by jachym, 6 years ago

Attachment: shp-geopackage.patch added

comment:1 by martinl, 6 years ago

Component: DefaultVector
Keywords: v.out.ogr added
Type: enhancementtask

comment:2 by neteler, 6 years ago

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?

in reply to:  2 comment:3 by mmetz, 6 years ago

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.

by jachym, 6 years ago

Attachment: shp-geopackage.2.patch added

Documentation updated too

comment:4 by jachym, 6 years ago

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.

comment:5 by jachym, 6 years ago

Any objections to svn commit ?

in reply to:  4 ; comment:6 by mlennert, 6 years ago

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 ?

in reply to:  6 comment:7 by mmetz, 6 years ago

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.

comment:8 by jachym, 6 years ago

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).

by jachym, 6 years ago

Attachment: shp-geopackage.3.patch added

Make sure, GPKG is available

comment:9 by rouault, 6 years ago

@jachym Your test logic is inverted ;-)

comment:10 by jachym, 6 years ago

shame on me :-(

Fixed / attached

by neteler, 6 years ago

Attachment: shp-geopackage.4.patch added

Patch updated with wording fixes in HTML file

comment:11 by neteler, 6 years ago

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

in reply to:  11 comment:12 by neteler, 6 years ago

 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 "

comment:13 by jachym, 6 years ago

@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?

by neteler, 6 years ago

Attachment: shp-geopackage.4.2.patch added

Updated patch

comment:14 by neteler, 6 years ago

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

comment:15 by rouault, 6 years ago

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 ?

comment:16 by rouault, 6 years ago

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

in reply to:  16 ; comment:17 by neteler, 6 years ago

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) :-)

by neteler, 6 years ago

Attachment: shp-geopackage.4.3.patch added

Updated patch

by mmetz, 6 years ago

Attachment: shp-geopackage.4.4.patch added

simplified patch

in reply to:  17 ; comment:18 by mmetz, 6 years ago

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.

comment:19 by jachym, 6 years ago

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

Big thanks for help!

comment:20 by neteler, 6 years ago

In 71597:

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

in reply to:  18 ; comment:21 by neteler, 6 years ago

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?

in reply to:  21 ; comment:22 by martinl, 6 years ago

Replying to neteler:

Backport or not?

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

comment:24 by wenzeslaus, 6 years ago

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

in reply to:  22 comment:25 by neteler, 6 years ago

Resolution: fixed
Status: newclosed

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.

comment:26 by sbl, 6 years ago

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...).

in reply to:  26 ; comment:27 by neteler, 6 years ago

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?

in reply to:  27 comment:28 by sbl, 6 years ago

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...

Note: See TracTickets for help on using tickets.