Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[build] Use a submodule for mapbox-gl-js rather than an npm dependency #7513

Merged
merged 1 commit into from
Dec 22, 2016

Conversation

jfirebaugh
Copy link
Contributor

Refs #5249

@mention-bot
Copy link

@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @tmpsantos, @incanus and @mourner to be potential reviewers.

@jfirebaugh
Copy link
Contributor Author

jfirebaugh commented Dec 21, 2016

See in particular #5249 (comment) for the rationale here.

@kkaefer @tmpsantos @1ec5 Any reservations about testing this out for a few days?

@1ec5
Copy link
Contributor

1ec5 commented Dec 21, 2016

I think it should be OK. We’re still using npm on the release-ios-v3.4.0 branch, which is going to be active for the next few weeks, with periodic merges back to master. It just means we have to be careful not to accidentally bring a submodule change along for the ride as we perform merges.

Copy link
Contributor

@tmpsantos tmpsantos left a comment

Choose a reason for hiding this comment

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

Thanks a lot for that!

@tmpsantos
Copy link
Contributor

tmpsantos commented Dec 22, 2016

Any reservations about testing this out for a few days?

No problem at all. Regarding the CI caching issue we could have a checkout of the repo on the travis cache directory and copy it to the submodule location on the travis shell script.

if(NOT EXISTS "mapbox-gl-js/package.json")
execute_process(
COMMAND git submodule update --init mapbox-gl-js
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
Copy link
Member

Choose a reason for hiding this comment

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

Can we try using externalproject_add with the GIT_SUBMODULES setting instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to get this to work. It looks to me like the GIT_SUBMODULES option is for initializing submodules of the external project, rather than using a submodule as the method to obtain the external project itself.

Copy link
Contributor

@tmpsantos tmpsantos Dec 23, 2016

Choose a reason for hiding this comment

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

I've been playing with externalproject_add for a while on the CMake build I'm maintaining for building Mapbox GL Native 100% offline and for Qt + Windows. On the build file for predownloading all of our dependencies I'm doing something like:

include(ExternalProject)

# Macro for cloning and downloading our header-only dependencies
macro(add_dep package tag git)
    externalproject_add(${package}
        URL https://github.com/${git}/archive/${tag}.zip
        CONFIGURE_COMMAND ""
        BUILD_COMMAND ""
        INSTALL_COMMAND ""
    )

    add_dependencies(mbgl-core ${package})

    externalproject_get_property(${package} SOURCE_DIR)

    target_include_directories(mbgl-core PRIVATE ${SOURCE_DIR}/include)
    target_include_directories(qmapboxgl PRIVATE ${SOURCE_DIR}/include)

    set(${package}_SOURCE_DIR ${SOURCE_DIR})
endmacro()

Two things: It is a lot faster to get the tarball rather then cloning the repo and CMake will try to configure + build + install, but you can override it.

@jfirebaugh jfirebaugh force-pushed the submodule branch 2 times, most recently from 2b80772 to 5c7d3e7 Compare December 22, 2016 18:22
@jfirebaugh jfirebaugh merged commit 317b2bf into master Dec 22, 2016
@jfirebaugh jfirebaugh deleted the submodule branch December 22, 2016 23:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants