-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Enable npm to build mapbox-gl-native package from source #2352
Conversation
The previously encountered issues with this change were:
|
Is this worth digging into before bumping to |
Yeah, definitely. That depends on having geojsonvt as a mason dependency anyway right? |
Yea, only reason I pinned at |
Fixed Android builds, but the Linux |
58a646b
to
22d9e43
Compare
@mikemorris Uh oh! Did I |
8e5239d
to
0a4c663
Compare
@mikemorris This branch should good now. Let me know if you see anything that's not ship-shape! |
@mikemorris What's the status of #2346 (comment)? |
@jfirebaugh It only happened when falling back to a |
I thought we were purposefully forcing a source build for geojsonvt to avoid mapbox/mason#111?
I still see this in this branch. |
This is working now, would appreciate a review of mapbox/mason#115. Once the mason core pull request is merged, I'll merge mapbox/mason#116, then move this back to mason |
b158b91
to
c379079
Compare
Building on Linux with g++-4.9 locally, I'm hitting the follow compilation error. Possible this could be related @jfirebaugh?
|
c379079
to
bf7ecaf
Compare
Compilation error above seems to have gone away since rebasing onto latest
Seeing the following boost assertion failure under
|
Tracked this down to a The invalid boxes already exist in /cc @danpat |
|
As an aside, I tried rebasing this onto @jfirebaugh's annotation refactor, and building against GeoJSONVT 2.1.0, neither of which affected this bug. |
bf7ecaf
to
05f8038
Compare
Running against /cc @kkaefer |
Running against Was finally able to capture a backtrace of the crash with everything running against the new variant headers:
|
Wondering if there could be something going on with the usage of variants in |
@mikemorris In |
Thinking this isn't the problem since the Linux build works fine?
Added these to the mason build, no effect.
Dropped this from the mason build (was OS X only), FIXED IT OMFG YES. |
Actually fixing the hidden inline constructor instead of dropping |
Wow. It's pretty disturbing that a hidden symbol could cause crashes at runtime rather than a link error. @springmeyer Do you have any theories about how that could happen, or ideas for how to avoid it? |
Yeah that is surprising. I've never encountered that before. Perhaps what is happening is the missing symbol leads to some bogus fallback to another symbol. But really I have no idea. I will mention that I've found the |
I think it may be because the hidden symbol is only used inside a (optional?) variant? |
not sure @mikemorris - but I'll add that this is not actually a segfault style crash. Its the process being stopped on an assertion because |
Well yea, but there's silenty missing labels everywhere for some reason too. Tried adding
It doesn't seem to do anything when I stick it in |
Glad to hear you got this working @mikemorris! What's the big picture now? Are we almost ready to 🚢 this branch? |
|
FINALLY UNBLOCKED with #2584 merge!
|
7552ef3
to
bd519c3
Compare
@lucaswoj When building from source with
|
Uhh... for some reason the contents of |
Ugh. Opened npm/npm#10243 |
cccedab
to
b64ab4b
Compare
This reverts commit 311bf93. more explicit require paths in tests
So npm won't clobber binding.gyp, refs npm/npm#10243
We need to get this working for Android builds on OS X before merging this into master. #2344 should help us test our changes.
cc @mikemorris
ref #2348 #2237 #2288 #2349
Fixes #2226