search

CartoDB / mobile-external-libs

External libraries needed for CARTO Mobile SDK
5 stars 6 forks source link

use submodule when possible #3

Closed farfromrefug closed 3 years ago

farfromrefug commented 3 years ago

Right now the code from externals like valhalla is copied here. It makes it almost impossible to keep up to date with local modifs but also make it hard to create PR to add features to Valhalla. I have the issue right now where i have a local modif for valhalla (to bring pedestrian cost based on hills / road) which i can PR to valhalla (not easily at least). Wouldn't it be better to maintain a fork of valhalla with the carto hacks and use it here as a sub?

farfromrefug commented 3 years ago

@mtehver valhalla published a big update 3.1.0 which would be amazing to get. What s your process for upgrading 3rd parties? (merging is tough without submodules)

mtehver commented 3 years ago

@farfromrefug You are absolutely right, ideally every external dependency should be a fork of the original project and a separate submodule of the project. Doing it now would require a substantial amount of work, though.

Regarding Valhalla 3.10, it seems to be a rather major update. I will try to add it to the 4.4 version of the SDK, by first forking the Valhalla original repository and then merging the local changes.

farfromrefug commented 3 years ago

@mtehver then could at least valhalla become a submodule? as you will do the merge anyway... I am also asking because that would allow us to create PRs for Valhalla. Some people are asking about offline valhalla just like you did. We could end up having valhalla support DB like you do. And i have PRs too (pedestrian hill factor).

mtehver commented 3 years ago

@farfromrefug Yes, will do it this way. I will create a public fork of Valhalla, merge the local changes and then use it as a submodule from libs-external.

mtehver commented 3 years ago

Valhalla 3.1.1 is now included as submodule. I use a forked version of Valhalla, that includes some additional safety checks related to missing tile. Plus some hacks.

Most other large 3rd party dependencies are now also used as subprojects. Thus closing the issue.