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

Internal libraries are ignored #13

Closed
mloskot opened this issue Mar 8, 2017 · 10 comments
Closed

Internal libraries are ignored #13

mloskot opened this issue Mar 8, 2017 · 10 comments
Assignees

Comments

@mloskot
Copy link

mloskot commented Mar 8, 2017

CMakeLists.txt seems to require all GDAL dependencies as external libraries.
For example, if json-c, zlib, libtiff, etc. are missing, CMake terminates with failure.

That is not how, for instance, NMAKE build configuration works: even if no additional libraries are specified/enabled in nmake.opt, command nmake /f makefile.vc completes build with success. GDAL will include not features for which external deps are missing though.

IMO, CMake configuration should offer the same approach:

git clone .../gdal
mkdir build
cd build
cmake ..

and even if I have no deps installed, I should expect to be able to build GDAL, with some fetures, no?

@BishopGIS
Copy link
Contributor

BishopGIS commented Mar 9, 2017

I think about it. Also several users, which build GDAL in Windows, point on this.

But behaviour you mentioned have several problems:

  1. If user already have some library in his system, and CMake not find it, external one will be download, configured and build. In current lib_gdal implementation user can point to the right library or decide to use external one. This is more flexible.
  2. Current implementation behaviour is common for all platforms - Windows, Linux, Mac OS.
  3. You can configure via command line different options. The typical full build for Windows looks like:

cmake -DBUILD_SHARED_LIBS=ON -DWITH_EXPAT=ON -DWITH_EXPAT_EXTERNAL=ON -DWITH_GeoTIFF=ON -DWITH_GeoTIFF_EXTERNAL=ON -DWITH_ICONV=ON -DWITH_ICONV_EXTERNAL=ON -DWITH_JSONC=ON -DWITH_JSONC_EXTERNAL=ON -DWITH_LibXml2=ON -DWITH_LibXml2_EXTERNAL=ON -DWITH_PROJ4=ON -DWITH_PROJ4_EXTERNAL=ON -DWITH_TIFF=ON -DWITH_TIFF_EXTERNAL=ON -DWITH_ZLIB=ON -DWITH_ZLIB_EXTERNAL=ON -DWITH_JBIG=ON -DWITH_JBIG_EXTERNAL=ON -DWITH_JPEG=ON -DWITH_JPEG_EXTERNAL=ON -DWITH_JPEG12=ON -DWITH_JPEG12_EXTERNAL=ON -DWITH_LibLZMA=ON -DWITH_LibLZMA_EXTERNAL=ON -DWITH_GEOS=ON -DWITH_GEOS_EXTERNAL=ON -DWITH_PYTHON=ON -DWITH_PYTHON3=OFF -DWITH_SQLite3=ON -DWITH_SQLite3_EXTERNAL=ON -DWITH_PNG=ON -DWITH_PNG_EXTERNAL=ON -DWITH_CURL=ON -DWITH_CURL_EXTERNAL=ON -DWITH_OpenSSL=ON -DWITH_OpenSSL_EXTERNAL=ON -DENABLE_OZI=ON -DWITH_PostgreSQL=ON -DWITH_PostgreSQL_EXTERNAL=ON -DENABLE_NITF_RPFTOC_ECRGTOC=ON -DENABLE_HDF4=ON -DWITH_HDF4=ON -DWITH_HDF4_EXTERNAL=ON -DENABLE_ECW=ON -DWITH_ECW=ON -DWITH_ECW_EXTERNAL=ON -DENABLE_MRSID=ON -DWITH_MRSID=ON -DWITH_MRSID_EXTERNAL=ON -DGDAL_ENABLE_GNM=ON -G "Visual Studio 12 2013" -T "v120_xp" ../

Maybe worth adding some preset for Windows builds at the beginning of the root CMakeLists.txt with this options? And for such typical build the command may looks like:

cmake -DTYPICAL_BUILD=ON ../

@mloskot
Copy link
Author

mloskot commented Mar 9, 2017

  1. If user already have some library in his system, and CMake not find it, external one will be download, configured and build. In current lib_gdal implementation user can point to the right library or decide to use external one.
  • GDAL already includes essential dependencies, for the reason: 1) ensure the right supported version of a dep is used and no conflicts occur 2) user's convenience
  • The flexibility is good but it should be opt-in, not on by default. If I want it, then I use -DENABLE_GDAL_DEPS_DOWNLOAD=YES or similar.

This is more flexible.

  • The forced flexibility is bad!
  1. You can configure via command line different options. The typical full build for Windows looks like: (...)
  • That is typical for general CMake use case, but not for specific GDAL use case.
  • Asking GDAL users to use such ridiculously long CMake command line is, well, ridiculous. Especially, if they can just namke /f makefile.vc and be happy. This is NOT going to help selling CMake for GDAL at all.
  • CMake for GDAL should align with current GDAL setup. Users can now build GDAL without any dependencies, any downloads, any configuration setup, and the build with essential features enabled will succeed.
  • CMake configuration should not try to be more clever and complex than necessary.

Shortly, typical GDAL build with CMake on any platform should be this:

git clone .../gdal
mkdir build
cd build
cmake ..
apps/gdalinfo --version

That's it, nothing more required.

@BishopGIS BishopGIS self-assigned this Mar 9, 2017
@BishopGIS
Copy link
Contributor

Agreed with simplification. Will do it.
Not agree with internal copy of libraries in lib_gdal:

  1. Existed copies of internal libraries moved to separated repositories. This is one of borsch main ideas.
  2. Conflicts can be solved via unit testing (special repository already created).
  3. Also special python script created to get all borsch libraries via one simple line of code (https://github.com/nextgis-borsch/borsch/blob/master/opt/tools.py).
  4. As Even said there is extra work to support internal libraries in syncing in GDAL.
  5. I think via simplification if user will enter the only

git clone .../gdal
mkdir build
cd build
cmake ..
apps/gdalinfo --version

it will be no matter where internal libraries sources get (from one repository or more). And this is more close how maven or gradle works for java.

@mloskot
Copy link
Author

mloskot commented Mar 9, 2017

  1. Existed copies of internal libraries moved to separated repositories. This is one of borsch main ideas.

I was/am not aware of borsch principles. I take the CMake for GDAL as generic configuration, independent from any thirdparty platform, any package managers (conan.io, vcpkg, maven, etc.). I thought, CMake for GDAL is supposed to be drop-in replacement/alternative for NMAKE/Automake makefiles.

  1. Conflicts can be solved via unit testing (special repository already created).

I assume, such issue is external to CMake for GDAL.

  1. Also special python script created to get all borsch libraries via one simple line of code (https://github.com/nextgis-borsch/borsch/blob/master/opt/tools.py).

See my answer to 1.

  1. As Even said there is extra work to support internal libraries in syncing in GDAL.

Yes, it is, but

  • That work has been done and will continue to be performed as part of GDAL project anyway.
  • There is maintenance hassle on GDAL maintainers (Even), but relying only on external libraries is not a golden cure: 1) carries risks of incompatibilities 2) shifts the hassle to users and package maintaners

Besides, I haven't heard of any plans to get rid of maintaining internal libraries. Here we would have to ask @rouault to confirm any such plans.

I think via simplification if user will enter the only (...) it will be no matter where internal libraries sources get (from one repository or more). And this is more close how maven or gradle works for java.

  • I assume CMake for GDAL is not supposed to be package maintenance tool, but simply a build configuration. All the magic happening behind scenes, automatic downloads of dependencies, etc. should not be part of CMake for GDAL build configuration. Or, it should be clearly optional.
  • If GDAL hosts internal libraries inside its own source tree, CMake for GDAL should respect that and not maintain internal libraries externally.

I sense, CMake for GDAL tries to do too many things at ones.
Borsch seems to be packaging system. That is OK, and why not make it a client/user of CMake for GDAL,
instead of making CMake for GDAL part of Borsch system.

In other words, let borsch call cmake with long list of -D... options specifying where all GDAL deps provided by Borsch packages are located.

CMake for GDAL should be completely generic and independent from tools and ideas of Borcsh or any packageing system. CMake for GDAL should be plain CMake scripts which build GDAL and, optionally, allows users to tell CMake where some of the non-default more advanced dependencies are located (like ./configure --with-....

Please, don't get me wrong. I'm not saying nextgis-borsch does anything wrong. It does whatever it needs to do for nextgis-borsch. I just tried it out thinking/assuming it is generic CMake for GDAL configuration. It turns out it is not. It is "CMake for GDAL in Borsch". It works for you, no doubt, but it is not what general public of CMake & GDAL users expect.

My 2 cents

@rouault
Copy link

rouault commented Mar 9, 2017

Besides, I haven't heard of any plans to get rid of maintaining internal libraries. Here we would have to ask @rouault to confirm any such plans.

I just came to the project after the fact, but for me, those internal libraries are mostly for convenience of Windows users, for which no standardized installation tree exist. The incompatibility argument is not so true, since we allow also build against external libraries, which can be of different versions.

@mloskot
Copy link
Author

mloskot commented Mar 9, 2017

@rouault Do you plan to get rid of the internal libraries?

@rouault
Copy link

rouault commented Mar 9, 2017

I would be -0 on this

@mloskot
Copy link
Author

mloskot commented Mar 9, 2017

@rouault OK

@BishopGIS I expected different approach of CMake for GDAL. Depending on concepts of any packaging system in CMake configuration is no-go for me and, IMO, no fit for GDAL. But, I don't want to impose anything to your approach. I'll rather close the issue.

@mloskot mloskot closed this as completed Mar 9, 2017
@BishopGIS
Copy link
Contributor

@mloskot the CMake for GDAL project began as your expected, but than transformed to something more different, as you say It is "CMake for GDAL in Borsch". This is result of user needs for build with different configurations and options, mainly for Windows users which used to select some checkboxes in CMake GUI and all dependencies magically solved :).
On Borsch principles we crossplatform build GDAL, QGIS, PostGIS on Windows, Linux and MacOS. Also we broadly use it in our software.
Anyhow I'll modify root GDAL CMakeLists.txt to support such approach:

git clone .../gdal
mkdir build
cd build
cmake ..
apps/gdalinfo --version

This is good idea (#14).

@mloskot
Copy link
Author

mloskot commented Mar 9, 2017

@BishopGIS Yes, understood. Thanks for all the clarifications and the effort towards simplifications. I will try to help testing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants