Find GDAL_DATA using INST_DATA without execution GDALAllRegister

[Bishop]

Some programs use GDAL functions without register any driver. For example crssync from QGIS (​https://github.com/qgis/QGIS/blob/master/src/crssync/main.cpp). This program search SpatialReference definitions from various sources, include GDAL_DATA folder (gcs.csv, pcs.csv, vertcs.csv, compdcs.csv, geoccs.csv, epsg.wkt, esri_extra.wkt).

With self build GDAL on Linux there is no problem, as GDAL_DATA path will be: /usr/local/share/gdal

In CPLFinderInit function using such default paths:

1. /usr/share/gdal

2. /usr/local/share/gdal

If libgdal install from packages, the path to GDAL_DATA set to: /usr/share/gdal/<major>.<minor>

As result, crssync did not find GDAL_DATA and QGIS have empty srs.db.

I suggest to add few lines to CPLFinderInit (cpl_findfile.cpp):

static FindFileTLS* CPLFinderInit()

{
    FindFileTLS* pTLSData = CPLGetFindFileTLS();
    if( pTLSData != NULL && !pTLSData->bFinderInitialized )
    {
        pTLSData->bFinderInitialized = TRUE;
        CPLPushFileFinder( CPLDefaultFindFile );

        CPLPushFinderLocation( "." );

        if( CPLGetConfigOption( "GDAL_DATA", NULL ) != NULL )
        {
            CPLPushFinderLocation( CPLGetConfigOption( "GDAL_DATA", NULL ) );
        }
        else
        {
#ifdef INST_DATA
            CPLPushFinderLocation( INST_DATA );
#endif
#ifdef GDAL_PREFIX
  #ifdef MACOSX_FRAMEWORK
            CPLPushFinderLocation( GDAL_PREFIX "/Resources/gdal" );
  #else
            CPLPushFinderLocation( GDAL_PREFIX "/share/gdal" );
  #endif
#else
            CPLPushFinderLocation( "/usr/local/share/gdal" );
#endif

       }
    }
    return pTLSData;
}

I can add this code if there are no objections.

[Even Rouault]

Looks good to me, but you likely need to edit port/GNUmakefile as well, as in gcore/GNUmakefile, to define INST_DATA

[Bishop]

Done in r34346

[Kurt Schwehr]

Should we drop the fixed /usr/local/share/gdal now that it can be set via INST_DATA?

port/cpl_findfile.cpp

@@ -115 +115 @@
   #else
             CPLPushFinderLocation( GDAL_PREFIX "/share/gdal" );
   #endif
-#else
-            CPLPushFinderLocation( "/usr/local/share/gdal" );
 #endif
         }
     }

[Bishop]

I think no - this is one more path for search files. Also removing /usr/local/share/gdal may affect mingw or cygwin.

[Kurt Schwehr]

But, anything depending on that location could now just set INST_DATA. And isn't that being set now for all GNU Make based builds? It's now an even more confusing set of things going on with no documentation.

And, for my installs, I only want the GDAL_DATA env var, so I current need to patch out the /usr/local/share/gdal. Inspecting /usr/local/... triggers a container security violation and kills the process. I'm just about to put the data files in /vsimem for my setup.

find . -type f | egrep -v 'NEWS|iso8211|[a-z]/config' | xargs egrep '/usr/local/share|INST_DATA|DATADIR|DATAROOT'
./apps/GNUmakefile:	echo 'CONFIG_DATA="$(INST_DATA)"' >> gdal-config-inst
./configure:  --datadir=DIR           read-only architecture-independent data [DATAROOTDIR]
./configure:  --infodir=DIR           info documentation [DATAROOTDIR/info]
./configure:  --localedir=DIR         locale-dependent data [DATAROOTDIR/locale]
./configure:  --mandir=DIR            man documentation [DATAROOTDIR/man]
./configure:  --docdir=DIR            documentation root [DATAROOTDIR/doc/PACKAGE]
./configure.in:dnl /usr/local/share/gdal/man but we want man pages in /usr/local/man.
./gcore/gdaldrivermanager.cpp:/*      files, and so forth.  Use the INST_DATA macro (setup at         */
./gcore/gdaldrivermanager.cpp:#ifdef INST_DATA
./gcore/gdaldrivermanager.cpp:        CPLPushFinderLocation( INST_DATA );
./gcore/GNUmakefile:	$(CXX) -c $(GDAL_INCLUDE) $(CPPFLAGS) $(CXXFLAGS) -DINST_DATA=\"$(INST_DATA)\" \
./gdal.pc:CONFIG_INST_DATA=/Users/schwehr/src/gdal/inst/share/gdal/data
./gdal.pc:datadir=${CONFIG_INST_DATA}
./gdal.pc.in:datadir=${CONFIG_INST_DATA}
./GDALmake.opt:INST_DATA 	=	${prefix}/share/gdal
./GDALmake.opt.in:INST_DATA 	=	@datadir@
./GNUmakefile:	$(INSTALL_DIR) $(DESTDIR)$(INST_DATA)
./GNUmakefile:	for f in LICENSE.TXT data/*.* ; do $(INSTALL_DATA) $$f $(DESTDIR)$(INST_DATA) ; done
./GNUmakefile:	echo 'CONFIG_INST_DATA=$(INST_DATA)/data' >> gdal.pc
./makefile.vc:	-mkdir $(DATADIR)
./makefile.vc:	$(INSTALL) data\*.* $(DATADIR)
./makefile.vc:	$(INSTALL) LICENSE.TXT $(DATADIR)
./nmake.opt:!IFNDEF DATADIR
./nmake.opt:DATADIR = $(GDAL_HOME)\data
./ogr/ogr_fromepsg.cpp: * These support files are normally searched for in /usr/local/share/gdal
./ogr/ogr_srs_dict.cpp: * this results in searching /usr/local/share/gdal or somewhere similar.
./ogr/ogrsf_frmts/generic/GNUmakefile:CXXFLAGS :=     $(CXXFLAGS) $(SHADOW_WFLAGS) -DINST_DATA=\"$(INST_DATA)\"
./port/cpl_csv.cpp:    strcpy( pTLSData->szPath, "/usr/local/share/epsg_csv/" );
./port/cpl_findfile.cpp:#ifdef INST_DATA
./port/cpl_findfile.cpp:            CPLPushFinderLocation( INST_DATA );
./port/cpl_findfile.cpp:            CPLPushFinderLocation( "/usr/local/share/gdal" );
./port/GNUmakefile:CPPFLAGS	:= $(CPPFLAGS)	$(CURL_INC) $(XTRA_OPT) -DINST_DATA=\"$(INST_DATA)\"
./swig/java/javadoc.java: * These support files are normally searched for in /usr/local/share/gdal
./swig/perl/lib/Geo/GDAL.dox:# '/Resources/gdal'), or '/usr/local/share/gdal'. It is usually only

[Bishop]

Current commit didn't change previous behavior. Only add one more search path. The GDAL_DATA environment variable will work as usual.

The INST_DATA also used in GDALAllRegister. The GDAL user or developer who used GDAL in his(her) software don't need to set or change INST_DATA - this is internal thing of GDAL make scripts.

[Kurt Schwehr]

Bishop, My comment wasn't that the commit changed prior behavior. My question is, now that we have INST_DATA, dists can use that to set a path and it would be nice to have the ability for folks that want it to not have any paths built in.

Distros could set GDAL_PREFIX to be /usr/local and they would get the same behavior as the final else with /usr/local/share/gdal, so why default to adding a path?

And what happens on a native Windows build? If no GDAL_PREFIX is set, does it go looking in /usr/local/share/gdal? Or GDAL_PREFIX "/share/gdal"? That appears to make no sense. What does Windows do with "/" rather than "\" these days? (No, I haven't gone looking for how the VC builds do this).

What am I missing? Why is it not a good idea to remove those two lines? That is a behavior change, but I think it's probably a good one. I'm not in any way arguing against the prior patch, which seems reasonable to me. I'm looking for ways that I can minimize the local patches I have to make to GDAL for my prod env without impacting others and I think that having GDAL_PREFIX = /usr/local gives the same behavior.

       if( CPLGetConfigOption( "GDAL_DATA", NULL ) != NULL )
        {
            CPLPushFinderLocation( CPLGetConfigOption( "GDAL_DATA", NULL ) );
        }
        else
        {
#ifdef INST_DATA
            CPLPushFinderLocation( INST_DATA );
#endif
#ifdef GDAL_PREFIX
  #ifdef MACOSX_FRAMEWORK
            CPLPushFinderLocation( GDAL_PREFIX "/Resources/gdal" );
  #else
            CPLPushFinderLocation( GDAL_PREFIX "/share/gdal" );
  #endif
#else
            CPLPushFinderLocation( "/usr/local/share/gdal" );
#endif
        }

[Bishop]

I don't understand that prevents to leave this line? Yes in some cases the INST_DATA may be /usr/local/share/gdal and we will have duplicated CPLPushFinderLocation( "/usr/local/share/gdal" ); But it seems to me there is no problem here.

But removing CPLPushFinderLocation( "/usr/local/share/gdal" ); may affect some already configured building systems.

Is it ok such regress?

[Even Rouault]

Kurt, as far as I can see, the line CPLPushFinderLocation( "/usr/local/share/gdal" ); shouldn't be compiled since it is in the else clause of #ifdef GDAL_PREFIX (that is always defined in cpl_config.h, unless you do weird things...).

Dmitry, regarding the addition of CPLPushFinderLocation( INST_DATA ); I'm just thinking that if people call also GDALAllRegister(), then INST_DATA will be put twice, and thus if a file fails from being found, it will be probed twice. That's probably not a big deal, but perhaps it would be worth to make CPLPushFinderLocation() detect duplicates ?

[Bishop]

I also think about filter duplicates. Ok, I'll fix all this (filter out duplicates and remove the CPLPushFinderLocation( "/usr/local/share/gdal" );).

[Kurt Schwehr]

Kurt, as far as I can see, the line CPLPushFinderLocation( "/usr/local/share/gdal" ); shouldn't be compiled since it is in the else clause of #ifdef GDAL_PREFIX (that is always defined in cpl_config.h, unless you do weird things...).

I definitely do weird things:

• build with bazel
• explicitly do not set GDAL_PREFIX
• only compile the drivers that I know I need
• (on the todo list) push the GDAL_DATA files I need to /vsimem and point GDAL_DATA there
• run in a sandbox that kills the process if it tries to in any way touch any filesystem in a way that I did not explicitly allow for

There is currently no way to not have a finder location built in without patching the code. It's not the end of the world, but now that INST_DATA is there, I see a way that those of us doing the crazy stuff can have a solution without patching. Are distros explicitly leaving GDAL_PREFIX to be undefined and counting on /usr/local/share/gdal? I would guess that they typically set the prefix to be /usr

[Even Rouault]

Are distros explicitly leaving GDAL_PREFIX to be undefined

As soon as you use ./configure, which is what 99.999% use on Unix ;-), it will generate a cpl_config.h with a non-empty GDAL_PREFIX. I guess we could safely remove the CPLPushFinderLocation( "/usr/local/share/gdal" ) If that make things easier for you, you could just enclose all those CPLPushFinderLocation() call in a specific #ifndef NO_HARDCODED_FIND_LOCATION or whatever better name you may find.

[bishop]

In 34379:

Remove hardcoded find location (/usr/local/share/gdal)
Add check duplicates in CPLPushFinderLocation function
Fix #6543

[Bishop]

Done in r34379

[Kurt Schwehr]

r34653 added GDAL_NO_HARDCODED_FIND. Tested with gunit like this:

TEST(CplCsvTest, GDALDefaultCSVFilename) {
  // Files that do not exist.
  // nullptr not allowed as an arg.
  EXPECT_THAT(GDALDefaultCSVFilename(""), StartsWith("/"));
  EXPECT_THAT(GDALDefaultCSVFilename(""), EndsWith("/"));
  EXPECT_STREQ("does_not_exist", GDALDefaultCSVFilename("does_not_exist"));
  EXPECT_STREQ("/foo/bar.csv", GDALDefaultCSVFilename("/foo/bar.csv"));
  EXPECT_STREQ("../baz.csv", GDALDefaultCSVFilename("../baz.csv"));

  // Try a file that GDAL does have.
  EXPECT_THAT(GDALDefaultCSVFilename("gcs.csv"), StartsWith("/"));
  EXPECT_THAT(GDALDefaultCSVFilename("gcs.csv"), EndsWith("/gcs.csv"));

  // GDAL specifically searches for these two files that can cause trouble.
  EXPECT_STREQ("datum.csv", GDALDefaultCSVFilename("datum.csv"));
  EXPECT_THAT(GDALDefaultCSVFilename("gdal_datum.csv"), StartsWith("/"));
  EXPECT_THAT(GDALDefaultCSVFilename("gdal_datum.csv"),
              EndsWith("/gdal_datum.csv"));

  // Test in memory filesystem.
  const string filename("/vsimem/gdaldefaultcsvfilename.txt");

  // Does not exist.
  EXPECT_STREQ(filename.c_str(), GDALDefaultCSVFilename(filename.c_str()));

  VSILFILE *file = VSIFOpenL(filename.c_str(), "wb");
  const string data("foo");
  VSIFWriteL(data.c_str(), data.length(), 1, file);
  VSIFCloseL(file);

  EXPECT_THAT(GDALDefaultCSVFilename(filename.c_str()), StartsWith("/"));
  EXPECT_THAT(GDALDefaultCSVFilename(filename.c_str()),
              EndsWith(filename.c_str()));

  VSIUnlink(filename.c_str());
}