8000 don't pollute include dir with installed submodules by Vedingrot · Pull Request #4452 · mapnik/mapnik · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom

don't pollute include dir with installed submodules #4452

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

don't pollute include dir with installed submodules #4452

wants to merge 1 commit into from

Conversation

Vedingrot
Copy link

When install mapnik with bundled submodules it creates mapbox and
protozero directories which could conflict with previously installed.

@codecov Codecov
Copy link
codecov bot commented May 16, 2024 8000 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.76%. Comparing base (09067f5) to head (a8d6532).
Report is 61 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4452   +/-   ##
=======================================
  Coverage   73.75%   73.76%           
=======================================
  Files         525      525           
  Lines       33532    33532           
  Branches     4139     4140    +1     
=======================================
+ Hits        24733    24736    +3     
+ Misses       8797     8794    -3     
  Partials        2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hummeltech
Copy link
Contributor
hummeltech commented May 17, 2024 •
edited
Loading

I see what you mean, but if one already has those dependencies installed, wouldn't they just build Mapnik with the corresponding CMake options?

mapnik/CMakeLists.txt

Lines 38 to 41 in a8d6532

mapnik_option(USE_EXTERNAL_MAPBOX_GEOMETRY "Use a external mapnik/geometry.hpp. If off, use the submodule" OFF)
mapnik_option(USE_EXTERNAL_MAPBOX_POLYLABEL "Use a external mapnik/polylabel. If off, use the submodule" OFF)
mapnik_option(USE_EXTERNAL_MAPBOX_PROTOZERO "Use a external mapnik/protozero. If off, use the submodule" OFF)
mapnik_option(USE_EXTERNAL_MAPBOX_VARIANT "Use a external mapnik/variant. If off, use the submodule" OFF)

@Vedingrot
Copy link
Author

For example, include/mapnik may contains unsupported version of
dependencies that requires by other programs. Using submodules solves
this problem but they should be installed in a directory that "belongs"
only for mapnik because they will be used only by it.

@mathisloge
Copy link
Collaborator

I don't think that th 8000 is is really needed, since you should install mapnik in a extra dir, if you have conflicting versions of those, since you should never link those bits together.

I do have concernce regarding the integration, since the headers now need to be included as <mapnik/protozero/...> etc.

@Vedingrot
Copy link
Author

There is compiler flag -I"${includedir}/mapnik that has been added
in https://github.com/mapnik/mapnik/pull/4441/files so there's no
need to change headers.

@landryb
Copy link
landryb commented Jul 10, 2024

i have to agree with the PR here, one might have protozero installed systemwide, which isnt the same version as the one forked by mapbox.. osrm-backend also installs some mapbox headers under include/mapbox so they would conflict.

my solution is to pass -DCMAKE_INSTALL_INCLUDEDIR=/usr/local/include/mapnik so that everything mapnik-related is in its own dir..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Vedingrot @hummeltech @mathisloge @landryb
0