Skip to content

remove libgeotiff from Requires in .pc file#5737

Merged
chrstnbwnkl merged 11 commits intomasterfrom
cb-geotiff-pc
Nov 26, 2025
Merged

remove libgeotiff from Requires in .pc file#5737
chrstnbwnkl merged 11 commits intomasterfrom
cb-geotiff-pc

Conversation

@chrstnbwnkl
Copy link
Copy Markdown
Member

@chrstnbwnkl chrstnbwnkl commented Nov 24, 2025

Since libgeotiff only recently started shipping a package-config file, we shouldn't list it in our pkg-config's Require section. On Ubuntu 24.04 LTS configuration will always fail since pkg-config won't find it unless you provide a dummy .pc file (which is a cumbersome hack).

@nilsnolde
Copy link
Copy Markdown
Member

That being said, it's a private dependency anyway, so we shouldn't have it in our .pc in the first place, right? (in which case we should also remove the libtiff dep)

right, and a few more it seems: spatialite & openssl could be removed too

nilsnolde
nilsnolde previously approved these changes Nov 24, 2025
@chrstnbwnkl chrstnbwnkl changed the title remove libgeotiff from Requires in .pc file remove private dependencies from Requires in .pc file Nov 25, 2025
Comment on lines -26 to -31
if(ENABLE_DATA_TOOLS)
list(APPEND REQUIRES spatialite sqlite3 luajit geos openssl)
endif()
if(ENABLE_HTTP)
list(APPEND REQUIRES libcurl)
endif()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's too much now;) except for spatialite and openssl, they're exposed in our headers and needed to resolve libvalhalla dependencies.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Urgh your're right. None of these are actually exposed in our headers (anymore, sqlite3 and GEOS are now forward declared and liblua includes were removed from headers in #5466) but downstream projects still need to link against them anyway since they're runtime dependencies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I should return to a computer for review😄

@chrstnbwnkl chrstnbwnkl changed the title remove private dependencies from Requires in .pc file remove libgeotiff from Requires in .pc file Nov 26, 2025
@chrstnbwnkl
Copy link
Copy Markdown
Member Author

Okay, back to the beginning... just remove libgeotiff 😄

@nilsnolde nilsnolde self-requested a review November 26, 2025 15:25
Copy link
Copy Markdown
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

It's fine to merge if it blocks for some reason. I'll have a look at the .pc again next week, don't think we're really doing the right things there currently (before & after this PR).

@chrstnbwnkl chrstnbwnkl merged commit 2636c6d into master Nov 26, 2025
18 checks passed
@chrstnbwnkl chrstnbwnkl deleted the cb-geotiff-pc branch November 26, 2025 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants