Skip to content

Conversation

@nilason
Copy link
Contributor

@nilason nilason commented Jan 6, 2026

Alternative to #6869.

This is the first part of an update to add CMake support for g.extension, extracted here the CMake export target configuration files part, to simplify review .

In addition to the "pure" export, the following updates are added:

  • reworked version handling (to account for version suffixes)
  • changed some passing of relative paths to absolute in CMakeFiles

The targets exported are in the pattern GRASS::<grass-lib-name-in-lowercase>, e.g. GRASS::gis, GRASS::raster.

cc @m-kuhn

@nilason nilason added this to the 8.5.0 milestone Jan 6, 2026
@github-actions github-actions bot added raster Related to raster data processing database Related to database management libraries module CMake labels Jan 6, 2026
@nilason
Copy link
Contributor Author

nilason commented Jan 8, 2026

@m-kuhn Is there any possibility that you can test this PR soon, or will it be good for you to test later on from main when you find the time?

@m-kuhn
Copy link
Contributor

m-kuhn commented Jan 9, 2026

It builds well here, thanks

There is one problem remaining, that (absolute) include paths of dependencies are propagated as include paths for grass and end up in the exported config.
These include paths are already defined in their own targets (GDAL::GDAL and PROJ::proj which are linked):
https://github.com/nilason/grass/blob/19517b0949e33c756f24ce054a14e87797aef8b1/lib/proj/CMakeLists.txt#L23

As far as I can see, this line can just be removed.

vcpkg build output (with the line removed):

Packages installed in this vcpkg installation declare the following licenses:
GPL-2.0-or-later
grass provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(GRASS CONFIG REQUIRED)
  # note: 50 additional targets are not displayed.
  target_link_libraries(main PRIVATE GRASS::dgl GRASS::gis GRASS::lrs GRASS::rli)

Is it correct, that we need all targets exported (I just did vector, raster and gis)?

@nilason
Copy link
Contributor Author

nilason commented Jan 9, 2026

It builds well here, thanks

Thanks for testing, really appreciate it!

There is one problem remaining, that (absolute) include paths of dependencies are propagated as include paths for grass and end up in the exported config. These include paths are already defined in their own targets (GDAL::GDAL and PROJ::proj which are linked): https://github.com/nilason/grass/blob/19517b0949e33c756f24ce054a14e87797aef8b1/lib/proj/CMakeLists.txt#L23

As far as I can see, this line can just be removed.

The problem is, unfortunately, that the GRASS lib's API exposes API from a couple of third party libraries. See #5263 for the rationale behind that line. So a third party software linking to the GRASS vector library also need, for example, GDAL include path, but not necessarily need to link to GDAL. I'm not sure how to solve that differently.

vcpkg build output (with the line removed):

Packages installed in this vcpkg installation declare the following licenses:
GPL-2.0-or-later
grass provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(GRASS CONFIG REQUIRED)
  # note: 50 additional targets are not displayed.
  target_link_libraries(main PRIVATE GRASS::dgl GRASS::gis GRASS::lrs GRASS::rli)

Is it correct, that we need all targets exported (I just did vector, raster and gis)?

How about find_package(GRASS REQUIRED COMPONENTS vector raster gis)?

@m-kuhn
Copy link
Contributor

m-kuhn commented Jan 9, 2026

There is one problem remaining, that (absolute) include paths of dependencies are propagated as include paths for grass and end up in the exported config. These include paths are already defined in their own targets (GDAL::GDAL and PROJ::proj which are linked): https://github.com/nilason/grass/blob/19517b0949e33c756f24ce054a14e87797aef8b1/lib/proj/CMakeLists.txt#L23
As far as I can see, this line can just be removed.

The problem is, unfortunately, that the GRASS lib's API exposes API from a couple of third party libraries. See #5263 for the rationale behind that line. So a third party software linking to the GRASS vector library also need, for example, GDAL include path, but not necessarily need to link to GDAL. I'm not sure how to solve that differently.

I am do not completely understand the problem yet. These types are accessed through e.g. close_ogr.c or read_ogr.c but I didn't find in cmake, where this is built. If these files are compiled as part of vector, we also need to link to the gdal target. If they are not built, what's the reason for having these types used in the headers?

How about find_package(GRASS REQUIRED COMPONENTS vector raster gis)?

That's what I expect to use. Is there also a need for find_package(GRASS REQUIRED COMPONENTS dgl rls rli)? I am just wondering, I don't think it will hurt.

@nilason
Copy link
Contributor Author

nilason commented Jan 10, 2026

There is one problem remaining, that (absolute) include paths of dependencies are propagated as include paths for grass and end up in the exported config. These include paths are already defined in their own targets (GDAL::GDAL and PROJ::proj which are linked): https://github.com/nilason/grass/blob/19517b0949e33c756f24ce054a14e87797aef8b1/lib/proj/CMakeLists.txt#L23
As far as I can see, this line can just be removed.

The problem is, unfortunately, that the GRASS lib's API exposes API from a couple of third party libraries. See #5263 for the rationale behind that line. So a third party software linking to the GRASS vector library also need, for example, GDAL include path, but not necessarily need to link to GDAL. I'm not sure how to solve that differently.

I am do not completely understand the problem yet. These types are accessed through e.g. close_ogr.c or read_ogr.c but I didn't find in cmake, where this is built. If these files are compiled as part of vector, we also need to link to the gdal target. If they are not built, what's the reason for having these types used in the headers?

The GDAL (and PostgreSQL) types are part of struct Map_info, via struct Format_info:

struct Format_info fInfo;

Map_info is used widely in the code so at this point, close before release, I can't think of a non-trivial fix that would address this. (I'd love to be proven to the contrary:)

I'll see if I can find out a workaround to target_include_directories. I'm curious, is the absolute path in the target a problem/blocker from your point of view?

How about find_package(GRASS REQUIRED COMPONENTS vector raster gis)?

That's what I expect to use. Is there also a need for find_package(GRASS REQUIRED COMPONENTS dgl rls rli)? I am just wondering, I don't think it will hurt.

The Addons typically use bitmap btree2 dbmibase gis gmath gproj imagery linkm raster segment vector. The rest of the libraries are not very likely to be used, but as the targets are generated automatically, why not keep them.

exposed in GRASS API, enabling linking core modules to GRASS libraries
without adding unnecessary dependency targets.
@github-actions github-actions bot added the vector Related to vector data processing label Jan 12, 2026
@nilason
Copy link
Contributor Author

nilason commented Jan 12, 2026

With latest commit I have removed all target_include_directories(...PUBLIC ...) that added absolute include paths to export config files. I replaced them with a more general include_directories() which works fine for building the core modules without adding a bunch of dependency targets.

@nilason nilason requested a review from HuidaeCho January 12, 2026 10:18
@m-kuhn
Copy link
Contributor

m-kuhn commented Jan 12, 2026

@nilason this change will avoid the problem of having absolute include paths, but downstream projects should specify the dependency on gdal and proj manually (it will probably work without in most cases if gdal and proj are installed to system and share the include folder with grass or other already included libraries, but not if they have been installed into separate directories)

I haven't worked with this myself, but I could imagine that the following pattern will extract the include directory at configure time of the downstream library (and not at build time of grass):

target_include_directories(grass_gproj
  INTERFACE
    $<TARGET_PROPERTY:GDAL::GDAL,INTERFACE_INCLUDE_DIRECTORIES>
    $<TARGET_PROPERTY:PROJ::proj,INTERFACE_INCLUDE_DIRECTORIES>
)

@nilason
Copy link
Contributor Author

nilason commented Jan 12, 2026

I haven't worked with this myself, but I could imagine that the following pattern will extract the include directory at configure time of the downstream library (and not at build time of grass):

target_include_directories(grass_gproj
  INTERFACE
    $<TARGET_PROPERTY:GDAL::GDAL,INTERFACE_INCLUDE_DIRECTORIES>
    $<TARGET_PROPERTY:PROJ::proj,INTERFACE_INCLUDE_DIRECTORIES>
)

Thanks for the tip, I'll explore if/how this can be used...

@nilason
Copy link
Contributor Author

nilason commented Jan 12, 2026

I haven't worked with this myself, but I could imagine that the following pattern will extract the include directory at configure time of the downstream library (and not at build time of grass):

target_include_directories(grass_gproj
  INTERFACE
    $<TARGET_PROPERTY:GDAL::GDAL,INTERFACE_INCLUDE_DIRECTORIES>
    $<TARGET_PROPERTY:PROJ::proj,INTERFACE_INCLUDE_DIRECTORIES>
)

Thanks for the tip, I'll explore if/how this can be used...

Indeed, this works for core modules, should therefore work for downstream projects too (including Addons, with some modifications to #6878). Updated with d34c526.
@m-kuhn Thanks!

@nilason
Copy link
Contributor Author

nilason commented Jan 12, 2026

@HuidaeCho Could you please take a look at this and/or approve? Need this merged to continue with #6878.

Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unblocking you in case you have to wait too long. It looks generally fine when reading through, though I haven't tried to run it

@HuidaeCho
Copy link
Member

HuidaeCho commented Jan 13, 2026

@nilason How can I test it? g.extension in #6878?

@nilason
Copy link
Contributor Author

nilason commented Jan 13, 2026

@nilason How can I test it? g.extension in #6878?

Yes, that would be the easiest way. I just updated it with recent changes here.

@nilason nilason merged commit d2980a5 into OSGeo:main Jan 15, 2026
27 checks passed
@nilason nilason deleted the cmake_add_export_target branch January 16, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake database Related to database management libraries module raster Related to raster data processing vector Related to vector data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants