Page MenuHomePhabricator

Don't reference git masters in package.json
Closed, ResolvedPublic

Description

https://phabricator.wikimedia.org/diffusion/GMKT/browse/master/package.json;86354b89eda66e37e37f29769e9de3585d6f3b05$70 brings in the dependency

json
"kartotherian-core": "git+https://github.com/kartotherian/kartotherian-core.git",

Although I'm sure it worked when it was written, it's now broken because that repository contains @kartotherian/core, not kartotherian-core

This causes a fresh install to fail with

[2017-09-28T19:59:40.968Z] FATAL: kartotherian/18321 on pippin: Cannot find module 'kartotherian-core'

Event Timeline

Pnorman created this task.Sep 28 2017, 8:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 28 2017, 8:13 PM
Pnorman claimed this task.Sep 29 2017, 5:57 AM
Pnorman moved this task from Backlog to In progress on the Maps-Sprint board.

Looking through an install log, the following are pulling from git and have a commit. These are probably referencing master, but need checking

+-- xmldoc@0.3.1  (git+https://github.com/nyurik/xmldoc.git#3107ca5fe10227539299fb1bb82992d5b824bc37)
+-- wikimedia-mapdata@0.3.0  (git+https://gerrit.wikimedia.org/r/mapdata#ee3a9e5dcfb89cffc28acb24e2cc281931ceaf08)
+-- postgis-vt-util@0.3.0  (git+https://github.com/mapbox/postgis-vt-util.git#5a62bbd3478a52716209fcbd2c9c474dbab29152)
+-- osm-bright-fonts@1.0.3  (git+https://github.com/kartotherian/osm-bright.fonts.git#83fa7114a82dfec4a2ae966bba08afd1f0e4c894)
+-- kad@1.3.6  (git+https://github.com/gwicke/kad.git#936c91652d757ea6f9dd30e44698afb0daaa1d17)

PRs in for postgis-vt-util, osm-bright-fonts. I can't find kad used, so wonder if it's coming from service-runner. Opened T184431: Move kartotherian to xmldoc 1.x for xmldoc.

Still need to do wikimedia-mapdata after breakfast.

Pnorman moved this task from In progress to Needs review on the Maps-Sprint board.Jan 9 2018, 6:21 PM

wikimedia-mapdata is referencing a specific commit, so it's okay.

PRs merged, new versions of everything uploaded, but I don't have access to publish to npmjs.com for kartotherian or tilerator.

ping @Yurik about getting access to the kartotherian and tilerator projects on npmjs.

Yurik added a comment.Jan 17 2018, 9:27 PM

@Pnorman, I have added you to the kartotherian npmjs project a week ago. Were you not able to publish?

Yurik added a comment.Jan 17 2018, 9:58 PM

@Pnorman my apologies, added.

PRs in for postgis-vt-util, osm-bright-fonts. I can't find kad used, so wonder if it's coming from service-runner.

Yes, kad is a service-runner thing.

Mholloway claimed this task.Jul 3 2018, 4:44 PM
Pnorman moved this task from Needs review to In progress on the Maps-Sprint board.Jul 3 2018, 4:44 PM
Jhernandez triaged this task as High priority.Jul 3 2018, 4:44 PM

Change 443682 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/Kartographer@master] Use published version of @Wikimedia/mapdata

https://gerrit.wikimedia.org/r/443682

Change 443682 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] Use published version of @Wikimedia/mapdata

https://gerrit.wikimedia.org/r/443682

Change 444009 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/Kartographer@master] Use published wikimedia fork of Leaflet.Sleep

https://gerrit.wikimedia.org/r/444009

Mholloway edited projects, added Maps; removed Maps (Kartotherian).

Moving columns because this applies to both Karotherian and Kartographer.

Change 444009 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] Use published wikimedia fork of Leaflet.Sleep

https://gerrit.wikimedia.org/r/444009

Mholloway added a comment.EditedJul 11 2018, 8:30 PM

xmldoc (a dependency of @kartotherian/module-loader) is going to be tricky to update, and here's why:

The *Loader classes in module-loader expose an update method meant to allow updating the internal representation of a parsed XML or YAML file and returning the result as an XML or YAML string. xmldoc, which the XmlLoader class uses to parse XML docs, doesn't appear to be designed for manipulating parsed XML, but only for reading it, and its XMLDocument and XMLElement classes don't provide any public interface for updating their contents. This is currently worked around by manipulating the val property of xmldoc XMLElement objects of interest.[1] As of a later commit adding typed nodes, this approach will no longer work, as for at least some node types the XMLElement's toString function ignores the val value in favor of other internal properties.[2]

I think it's a mistake to rely on xmldoc's internal representation of an XML element as we're currently doing. As the current example shows, this ties us to a specific version of xmldoc essentially forever. We can't take advantage of the CDATA handling later added upstream, which was the original reason for forking, or any other updates.

Since xmldoc doesn't expose a public API for updating parsed XML contents, I think we have the wrong tool for the job. I'd recommend switching to a more full-featured library; libxmljs is the go-to in this area, and appears to provide the functionality we're looking for.

[1] https://github.com/kartotherian/module-loader/blob/7373f60cb6730e40a3c91e729fb095dd7672e7d4/lib/XmlLoader.js#L186
[2] https://github.com/nfarina/xmldoc/commit/8218fb22e1f12349fa12874d2a61dd2d83d19c2a

Has splitting the module-loader module into separate XML and YAML loaders (module-loader-xml and module-loader-yaml) been considered?

Mholloway added a comment.EditedJul 13 2018, 6:56 PM

Maybe a better question is this: Do we want kartotherian and tilerator to be able to continue to support XML definition files, or is it reasonable these days only to support YAML? What's the state of the art these days?

If we're ready to drop XML support, then we can split up module-loader, use only module-loader-yaml in kartotherian and tilerator (via core), and the xmldoc stuff mentioned above falls in priority to the point where we don't really have to worry about it anymore, though it would be a welcome volunteer contribution. If dropping XML support is too drastic a move, then we'll have to get to the xmldoc stuff soon.

Yurik added a comment.Jul 13 2018, 7:56 PM

xml is only needed to configure Mapnik - so if it is possible to use some standard component to convert yml->xml for it, it would work. I think the issue here is that Mapnik is doing some non-standard xml parsing - hence all the problems. @Pnorman would know this better.

I'd rather not drop XML support, because it's still used by some people to develop tiles, and very important for writing testcases.

For the styles we have, there's no need to have XML at rest and the conversion to XML is handled within tilelive-tmsource and tilelive-tmstyle

PR to switch to libxmljs: https://github.com/kartotherian/module-loader/pull/2

I believe that's the last git master reference at issue here. (As noted above there's also kad coming from service-runner, which I'll have to bug the services team about sometime, but that seems out of scope.)

Mholloway closed this task as Resolved.Jul 21 2018, 2:43 PM