Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require Mapnik 3 and CartoCSS 0.16.0 #2473

Merged
merged 3 commits into from
Dec 4, 2016
Merged

Conversation

pnorman
Copy link
Collaborator

@pnorman pnorman commented Nov 28, 2016

This should wait until after v2.45.0.

Fixes #2458, #2198.

Enables #2470, #2045

@pnorman
Copy link
Collaborator Author

pnorman commented Nov 28, 2016

carto requires a .mml suffix. I could rename project.yaml to project.mml, but I'm reluctant because that would create a conflict in git for every existing PR that touches the layers.

@sommerluk
Copy link
Collaborator

I don’t understand well the mml/yaml/carto problem. Could you explain this further?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 28, 2016

Removing yaml2mml.py script in this PR also resolves #2447.

[EDIT:] It will also invalidate #2459.

@jojo4u
Copy link

jojo4u commented Nov 28, 2016

carto requires a .mml suffix. I could rename project.yaml to project.mml, but I'm reluctant because that would create a conflict in git for every existing PR that touches the layers.

Quick count: 5 of the 16 other PRs touch project.yaml: #2472 #2385 #2370 #2361 #2279. Are other affected? Changing the PRs would not be asking too much imho. Probably just a local rename of project.yaml to project.mml?

@kocio-pl
Copy link
Collaborator

#2423 would be probably also resolved by upgrading the minimum required version of CartoCSS with this PR.

@jojo4u
Copy link

jojo4u commented Nov 28, 2016

[EDIT:] It will also invalidate #2459.

No problem with dropping the script, the PR was good for learning python some more.

@pnorman
Copy link
Collaborator Author

pnorman commented Nov 28, 2016

I don’t understand well the mml/yaml/carto problem. Could you explain this further?

carto requires the MML file to have the suffix .mml

@nebulon42
Copy link
Contributor

I left the extension at .mml to keep the name. It just does not care anymore if it is JSON or YAML. An upstream change would also be possible but I'd rather only do this if absolutely necessary.

@@ -9,10 +9,8 @@ addons:
node_js:
- "0.10"
install:
- npm install carto@0.12.1
- npm install carto@0.16.0
Copy link
Contributor

Choose a reason for hiding this comment

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

You should really use 0.16.3 as the versions before contain some regressions. For linting this does not make a difference but maybe is the wrong signal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is in sync with with INSTALL.md. I want to test with the minimum carto version and the latest, but that's a bigger change that I want to keep to a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@kocio-pl
Copy link
Collaborator

I left the extension at .mml to keep the name. It just does not care anymore if it is JSON or YAML. An upstream change would also be possible but I'd rather only do this if absolutely necessary.

Why? I think files recognized with their proper extensions would be the most natural solution (and .mml covering all as a fallback).

@nebulon42
Copy link
Contributor

In this case there is no proper extension. There is no convention that YAML files should be named .yml or .yaml. The current JSON is also not named .json. I don't know why Mapbox chose .mml and what it means. Maybe Map(box) markup language or something. The only change I see would be that Carto does no longer care about the extension at all.

@gravitystorm
Copy link
Owner

I don't know why Mapbox chose .mml and what it means.

Fun fact - .mml comes from Cascadenik, a predecessor to CartoCSS created outside of Mapbox. In Cascadenik it contained XML.

@kocio-pl
Copy link
Collaborator

Quick count: 5 of the 16 other PRs touch project.yaml: #2472 #2385 #2370 #2361 #2279. Are other affected? Changing the PRs would not be asking too much imho.

So, if MML doesn't mean anything special, let's make it The Right Way (TM). Three of these PRs are mine and I'm willing to fix them (except one which is probably going to waste anyway).

No problem with dropping the script, the PR was good for learning python some more.

So was my Bash code fix, which I've just dropped. 😄

@pnorman
Copy link
Collaborator Author

pnorman commented Dec 1, 2016

If we decide to merge this I'll probably just take the first commit, and redo the delete and rename instead of resolving merge conflicts.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 2, 2016

As far as I understand, we're positive about merging it.

The only problem I see is release policy. We should make a bugfix release very soon and it would be v2.45.1 then, but if we merge also this code, this would be v3.0.0 probably, which would break our typical release time frame. Which solution do you think would be better?

@matthijsmelissen
Copy link
Collaborator

We could release 2.45.1 and 3.0.0 at the same time, and leave it to the server admins which version to install.

@kocio-pl kocio-pl mentioned this pull request Dec 2, 2016
@pnorman
Copy link
Collaborator Author

pnorman commented Dec 2, 2016

Merging this code doesn't mean we need to immediately do a release

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 3, 2016

It also enables #1126.

@pnorman
Copy link
Collaborator Author

pnorman commented Dec 3, 2016

Consensus seems to be we're ready to merge. I'll rebase later today.

@HolgerJeromin
Copy link
Contributor

It also enables #1126.

No, osm fr server do not run mapnik 3.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 3, 2016

So the milestone label for it was misleading, I've changed it now.

@nebulon42
Copy link
Contributor

Then I can merge it tomorrow (CET).

This
- drops Tilemill support;
- increments the major version; and
- removes some additional requirements for non-Latin languages
With CartoCSS 0.16.0 required, we don't need to create JSON anymore.
This simplifies the contribution process for layer editing.
@pnorman pnorman merged commit ba2ddd6 into gravitystorm:master Dec 4, 2016
@pnorman pnorman deleted the mapnik-3 branch December 4, 2016 07:45
@pnorman pnorman restored the mapnik-3 branch December 4, 2016 08:03
@matthijsmelissen
Copy link
Collaborator

Small side-question: since I upgraded kosmtik and carto 0.16, my cache doesn't flush anymore, which is quite problematic when developing. Is this a known problem with the last version(s)? Is there a way to flush the cache manually?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 5, 2016

You mean kosmtik/tmp/? Each project keeps its cache files under separate directory.

@pnorman
Copy link
Collaborator Author

pnorman commented Dec 5, 2016

Small side-question: since I upgraded kosmtik and carto 0.16, my cache doesn't flush anymore, which is quite problematic when developing

My startup line is rm -rf tmp/ && ~/osm/kosmtik/index.js serve --host 192.168.1.1 project.yml when working on vector tile stuff where I need to get rid of the cache all the time.

@matthijsmelissen
Copy link
Collaborator

Thanks. Strange though, the cache used to be invalidated every time project files changed on disk. Removing tmp every time I change a file is quite cumbersome.

@imagico
Copy link
Collaborator

imagico commented Dec 5, 2016

Having started using kosmtik not too long ago i had the impression this was normal but now that you mention it it seems indeed rather odd.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 5, 2016

I started to clean the Kosmtik cache from some moment, so it probably worked before. We have a fresh ticket about this problem now:

kosmtik/kosmtik#191

@jojo4u
Copy link

jojo4u commented Dec 5, 2016

I'm in same situation as imagico.

@nebulon42
Copy link
Contributor

nebulon42 commented Dec 5, 2016

I'm pretty sure that this is unrelated to carto 0.16.x.

@Ircama
Copy link
Contributor

Ircama commented Dec 6, 2016

Just wondering whether there is a reason why the latest YAML format of project.mml has the same basename of the Tilemill JSON format.

AFAIK Tilemill for Windows does not allow to change the project filename. Even if Tilemill is no more supported now, one can still try to keep on using it and in case there is the need to rename project.mml to project.yaml, restore yaml2mml.py (or equivalent command) and generate project.mml in JSON format: this might lead to errors/confusion with the original YAML file. Wouldn’t be more appropriate naming the new project.mml (YAML) to osm-carto.mml or similar?

@pnorman
Copy link
Collaborator Author

pnorman commented Dec 6, 2016

If someone is using tilemill I believe they need to be using a version from source to get an appropriate Mapnik version, which should also have a CartoCSS version which supports reading YAML. They'll have problems editing the layers through Tilemill, but given Tilemill's interface, there's problems doing any editing through it.

@Ircama
Copy link
Contributor

Ircama commented Dec 6, 2016

If someone is using tilemill I believe they need to be using a version from source to get an appropriate Mapnik version, which should also have a CartoCSS version which supports reading YAML.

I haven't checked the Ubuntu version yet. I am afraid that with the Windows version (which is the only native way to run openstreetmap-carto with Windows and without an Ubuntu VM - just run, not edit) there is no possibility to perform a manual update (and manually updating node_modules\carto does not provide a working product).

@pnorman
Copy link
Collaborator Author

pnorman commented Dec 6, 2016

I haven't checked the Ubuntu version yet. I am afraid that with the Windows version (which is the only native way to run openstreetmap-carto with Windows and without an Ubuntu VM - just run, not edit) there is no possibility to perform a manual update (and manually updating node_modules\carto does not provide a working product).

My point is that you can't use the prebuilt tilemill binaries, since we require Mapnik 3, and the version of Mapnik that came with v0.10.1 is from 2012, so we don't need to worry if something else is broken on Tilemill v0.10.1

Ircama added a commit to Ircama/openstreetmap-carto that referenced this pull request Dec 7, 2016
Rename project.yaml to project.mml following gravitystorm#2473
sommerluk pushed a commit to sommerluk/openstreetmap-carto that referenced this pull request Dec 17, 2016
Rename project.yaml to project.mml following gravitystorm#2473
@pnorman pnorman deleted the mapnik-3 branch December 20, 2016 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants