mapnik / python-mapnik

Python bindings for mapnik
GNU Lesser General Public License v2.1
160 stars 91 forks source link

Update to mapnik 3.0.13 mason package for travis builds #143

Closed springmeyer closed 7 years ago

springmeyer commented 7 years ago

https://github.com/mapbox/mason/pull/365 is available now in mason. Next step is to point the python-mapnik builds at it. The goal here is to move to testing python-mapnik master against a stable mapnik version (rather than a moving target).

/cc @artemp @flippmoke

springmeyer commented 7 years ago

Started this in https://github.com/mapnik/python-mapnik/tree/stable-upstream-3.0.13, however I'm blocked on:

src/mapnik_datasource.cpp:37:10: fatal error: 'mapnik/geometry/box2d.hpp' file not found
#include <mapnik/geometry/box2d.hpp>
         ^
1 error generated.

@artemp can you please either:

1) branch to create a python-mapnik that works against Mapnik 3.0.x, or 2) Ideally, fix master to work with both 3.1.x and 3.0.x like @flippmoke did at https://github.com/mapbox/mapnik-vector-tile/pull/227/commits/4ef99132510ef4a3a4283f9dd9ccec0cdd55d798

artemp commented 7 years ago

@springmeyer - there is already https://github.com/mapnik/python-mapnik/tree/v3.0.x

springmeyer commented 7 years ago

Thanks @artemp for making a v3.0.13 tag and helping me notice the v3.0.x branch. I've re-applied the pinned mason package against the v3.0.x branch. All tests pass except one - https://github.com/mapnik/python-mapnik/issues/139 - do you know what changed?

artemp commented 7 years ago

All tests pass except one - #139 - do you know what changed?

According to https://tools.ietf.org/html/rfc7946#section-3.1.1 :

A position is an array of numbers.  There MUST be two or more
elements.  The first two elements are longitude and latitude, or
easting and northing, precisely in that order and using decimal
numbers.  Altitude or elevation MAY be included as an optional third
element.
"coordinates": [ [ [ [] ] ] ] 

The above is a valid JSON but not valid GeoJSON (as per spec ^)

/cc @springmeyer

springmeyer commented 7 years ago

@artemp okay, so do the test expectations need updated? If so, can you push the correct update to master and the v3.0.x branch?

artemp commented 7 years ago

@springmeyer - ^ yes! This issue surfaced in node-mapnik https://github.com/mapnik/node-mapnik/pull/729 but test coverage there is somewhat limited.

I'm going to make some changes to make handling empty geometries more consistent, because following GeoJSON should also throw invalid dataset exception:

{ "type": "Feature", "properties": { }, "geometry": { "type": "Point", "coordinates": [] }}
artemp commented 7 years ago

I'm working on this PR https://github.com/mapnik/mapnik/pull/3643 /cc @springmeyer

springmeyer commented 7 years ago

Original issue solved in #144