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

Windows: failing paths, and filenames tests, that don't fail #517

Closed
wilhelmberg opened this issue Sep 9, 2015 · 5 comments
Closed

Windows: failing paths, and filenames tests, that don't fail #517

wilhelmberg opened this issue Sep 9, 2015 · 5 comments

Comments

@wilhelmberg
Copy link
Contributor

https://ci.appveyor.com/project/Mapbox/node-mapnik/build/1.0.777/job/qaxj7ku9ookcux2y#L2960

image

@springmeyer
Copy link
Member

Interesting problem. I see that the test this is coming from has not been checking the resulting image (https://github.com/mapnik/node-mapnik/blob/lazy-pbf/test/unicode-loading.test.js#L75-L76). So, it is possible this has been failing silently until now and that the only thing that changed is that recent mapnik added the "SVG PARSING ERROR". If that hunch is correct then loading of SVG files from unicode paths may have always been broken on windows.

@springmeyer
Copy link
Member

@BergWerkGIS - yes, looks like this is broken in Mapnik.

Can you try this patch:

diff --git a/src/svg/svg_parser.cpp b/src/svg/svg_parser.cpp
index 06c7202..ec1f4d8 100644
--- a/src/svg/svg_parser.cpp
+++ b/src/svg/svg_parser.cpp
@@ -28,6 +28,7 @@
 #include <mapnik/safe_cast.hpp>
 #include <mapnik/svg/svg_parser_exception.hpp>
 #include <mapnik/util/file_io.hpp>
+#include <mapnik/util/utf_conv_win.hpp>
 #include "agg_ellipse.h"
 #include "agg_rounded_rect.h"
 #include "agg_span_gradient.h"
@@ -974,7 +975,11 @@ svg_parser::~svg_parser() {}

 bool svg_parser::parse(std::string const& filename)
 {
+#ifdef _WINDOWS
+    std::basic_ifstream<char> stream(mapnik::utf8_to_utf16(filename));
+#else
     std::basic_ifstream<char> stream(filename.c_str());
+#endif
     if (!stream)
     {
         std::stringstream ss;

Also we need to start actually testing that the expected image in this test matches a known fixture. Can you add that to node-mapnik?

@wilhelmberg
Copy link
Contributor Author

@springmeyer
Looking much better now:

image

Leaving open, till mapnik/master is patched, too.


Take away:
Investigate tests, that take a "long" time.
See screenshot in OP: 50ms
That's gone now, too.

@springmeyer
Copy link
Member

Leaving open, till mapnik/master is patched, too.

Great, can you: 1) create a pull request at mapnik/mapnik, 2) close this ticket and link to new pull, 3) assign that pull to next 3.0.x milestone, 4) merge it once travis passes, 5) add a note about the fix to the CHANGELOG. Then the fix will go into the next release of Mapnik /cc @artemp @flippmoke

@wilhelmberg
Copy link
Contributor Author

Will be fixed in next mapnik release
mapnik/mapnik#3063

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

No branches or pull requests

2 participants