Mapbox-gl-native: Use Git submodules for shaders/test suite

[kkaefer]

@jfirebaugh, I'm frequently running into issues with npm not updating those two repositories. What are the reasons we use npm to manage these rather than just using submodules?

[jfirebaugh]

In my experience, updating dependencies are more of a hassle with submodules than with node packages. All the make commands have dependencies that should cause npm update to be run whenever package.json is more recent than node_modules. I don't know of a similar way to get that behavior with submodules.

What situation are you seeing where node_modules doesn't get updated?

[tmpsantos]

What situation are you seeing where node_modules doesn't get updated?

For instance depending on npm will make the Yocto and Buildroot packages extra complicated, because you will have npm as buildtime dependencies for embedded systems. I second using git submodules.

[kkaefer]

I'm using linked npm packages, and npm update doesn't work with that. Is there another way of updating the code of a package except for linking it?

[jfirebaugh]

A submodule might work for shaders, if we can find a way to auto-update it like we do now via npm update. I don't think a submodule is workable for the test-suite, because it has sub-dependencies that need to be initialized by npm.

[tmpsantos]

I don't think a submodule is workable for the test-suite, because it has sub-dependencies that need to be initialized by npm.

From embedded packaging perspective that would be fine. We don't build the tests when making the packages.

[kkaefer]

I've found that when the submodule rev changed on a certain parent module commit, it automatically gets checked out to that version when you change the HEAD. Apart from that, we can use this combination:

.gitmodules:

[submodule "deps/shaders"]
    path = deps/shaders
    url = https://github.com/mapbox/mapbox-gl-shaders
    ignore = dirty

and calling git submodule update --init -- deps/shaders to force the submodule to the correct pinned commit.

[jfirebaugh]

I've found that when the submodule rev changed on a certain parent module commit, it automatically gets checked out to that version when you change the HEAD.

Are you testing that in GitUp? It's a feature of that app, but not the git checkout command.

[kkaefer]

I used the CLI, but I had GitUp open with that repo, so maybe it did that in the background.

[tmpsantos]

Any decision on this? Can we move forward on not depending on npm for building on Linux?

[jfirebaugh]

• mapbox-gl-test-suite -- not feasible not to use npm due to subdependencies.
• mapbox-gl-shaders -- I'm fine with using a submodule if someone does the work to ensure that make commands automatically update it if it's out of date. I'm not planning to work on this myself.

[tmpsantos]

Depends on #5359. We can simple use externalproject_add(GIT_SUBMODULES).

https://cmake.org/cmake/help/v3.0/module/ExternalProject.html

[kkaefer]

I've found that using npm with git creates a lot of duplicate clones in ~/.npm; they took up several gigabytes on my machine.

[kkaefer]

I tried using ExternalProject_Add, but found odd behavior: It always runs the configure/build/install steps, regardless of whether any code was updated. This is unfortunate because it means that we'd run npm install et al every time a build runs, even though we only rarely update it.

[mikemorris]

I've found that using npm with git creates a lot of duplicate clones in ~/.npm; they took up several gigabytes on my machine.

This is by design in npm v2, and theoretically fixed in npm v3, but the implementation is still buggy 😞 https://github.com/npm/npm/issues/10727

[tmpsantos]

I tried using ExternalProject_Add, but found odd behavior: It always runs the configure/build/install steps, regardless of whether any code was updated.

It is possible to workaround this by adding empty steps. A header only library would look like:

externalproject_add(geojson-cpp
    URL https://github.com/mapbox/geojson-cpp/archive/v0.1.4.zip
    CONFIGURE_COMMAND ""
    BUILD_COMMAND ""
    INSTALL_COMMAND ""
)

add_dependencies(mbgl-core geojson-cpp)
externalproject_get_property(geojson-cpp SOURCE_DIR)
target_include_directories(mbgl-core PRIVATE ${SOURCE_DIR}/include)

[kkaefer]

Yeah, I did that as well, but ideally, we'd want to run npm install as the INSTALL_COMMAND, but not every time you run cmake, only when the actual source changes.

[jfirebaugh]

We've encountered a couple issues with npm that encourage switching to a submodule:

• npm incorrectly dedupes git dependencies pinned to different hashes. @boundsj encountered this on the dds-wip branch yesterday: mapbox-gl-style-spec is pinned to different commits in mapbox-gl-native package.json versus mapbox-gl-js package.json, and npm installed the mapbox-gl-js version at the top level. This will cease to be an issue once we have a full mapbox-gl-js monorepo, but it's an issue right now.
• npm (correctly) omits files specified via .npmignore even for git dependencies. This is an issue because mapbox-gl-js needs to exclude the test directory from the published package, but mapbox-gl-native wants that directory because it's where the test-suite integration tests are going to live (https://github.com/mapbox/mapbox-gl-js/pull/3834).

Additionally, with a mapbox-gl-js monorepo, it seems likely that a submodule will perform better for day-to-day development than an npm dependency:

• Updates are efficient get fetch operations, rather than complete fresh clones.
• You can work on dependent changes directly in the submodule, and push them to the mapbox-gl-js origin, rather than doing an npm link dance.

As noted above, we'll need to cope with subdependencies of the submodule, which will no longer be installed automatically by npm. We can do this by having the build scripts run npm install inside the submodule. Conveniently, this will allow mapbox-gl-js to use dev dependencies for the test-suite harness, which wouldn't be installed if mapbox-gl-js were itself an npm dependency of mapbox-gl-native.

[jfirebaugh]

We can do this by having the build scripts run npm install inside the submodule.

A drawback to this solution is that we're unable to cache mapbox-gl-js/node_modules using Travis's caching mechanism. I tried adding that directory to the caching list, but then initializing the submodule fails because the mapbox-gl-js directory is non-empty.

Not having it cached seems to increase the build times substantially.

[kkaefer]

As @tmpsantos suggests in https://github.com/mapbox/mapbox-gl-native/pull/7513#issuecomment-268690381, can we create a symlink of mapbox-gl-js/node_modules to the root node_modules folder after we do the initial checkout and before running npm install?

[jfirebaugh]

Clever! I'll try it.

[jfirebaugh]

Done in #7531.