WIP Added Windows Platform

[sopyer]

Based on work of Mathis Logemann mathisloge@gmail.com and KaiN kain@piwnica.ws
Fully compiles and run with opening folder in VS. Requires vcpkg as package manager.
Patch for geojson-vt-cpp submodule:

 geometry.hpp/include/mapbox/geometry/feature.hpp  | 5 +++++
 geometry.hpp/include/mapbox/geometry/geometry.hpp | 6 ++++++
 include/mapbox/geojsonvt/tile.hpp                 | 6 +++---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/geometry.hpp/include/mapbox/geometry/feature.hpp b/geometry.hpp/include/mapbox/geometry/feature.hpp
index 7008198..9af80e9 100644
--- a/geometry.hpp/include/mapbox/geometry/feature.hpp
+++ b/geometry.hpp/include/mapbox/geometry/feature.hpp
@@ -37,7 +37,12 @@ using value_base = mapbox::util::variant<null_value_t, bool, uint64_t, int64_t,
 
 struct value : value_base
 {
+#ifdef _MSC_VER
+    template< typename T >
+    value(T&& val): value_base(val) {}
+#else
     using value_base::value_base;
+#endif
 };
 
 using property_map = std::unordered_map<std::string, value>;
diff --git a/geometry.hpp/include/mapbox/geometry/geometry.hpp b/geometry.hpp/include/mapbox/geometry/geometry.hpp
index a9d072b..d6c6716 100644
--- a/geometry.hpp/include/mapbox/geometry/geometry.hpp
+++ b/geometry.hpp/include/mapbox/geometry/geometry.hpp
@@ -31,7 +31,13 @@ template <typename T>
 struct geometry : geometry_base<T>
 {
     using coordinate_type = T;
+
+#ifdef _MSC_VER
+    template <typename U>
+    geometry(U&& val): geometry_base<T>(val) {}
+#else
     using geometry_base<T>::geometry_base;
+#endif
 
     /*
      * The default constructor would create a point geometry with default-constructed coordinates;
diff --git a/include/mapbox/geojsonvt/tile.hpp b/include/mapbox/geojsonvt/tile.hpp
index b1e16a1..875f7fa 100644
--- a/include/mapbox/geojsonvt/tile.hpp
+++ b/include/mapbox/geojsonvt/tile.hpp
@@ -96,19 +96,19 @@ private:
     }
 
     void addFeature(const vt_point& point, const property_map& props, const identifier& id) {
-        tile.features.push_back({ transform(point), props, id });
+        tile.features.emplace_back( transform(point), props, id );
     }
 
     void addFeature(const vt_line_string& line, const property_map& props, const identifier& id) {
         const auto new_line = transform(line);
         if (!new_line.empty())
-            tile.features.push_back({ std::move(new_line), props, id });
+            tile.features.emplace_back( std::move(new_line), props, id );
     }
 
     void addFeature(const vt_polygon& polygon, const property_map& props, const identifier& id) {
         const auto new_polygon = transform(polygon);
         if (!new_polygon.empty())
-            tile.features.push_back({ std::move(new_polygon), props, id });
+            tile.features.emplace_back( std::move(new_polygon), props, id );
     }
 
     void addFeature(const vt_geometry_collection& collection, const property_map& props, const identifier& id) {

TODO:

1. Cleanup, further patch reduction
2. Fix 403 HTTP error
3. Tests
4. Verify other platforms.

[tehKaiN]

Let's do this already!

I've noticed some problems on my latest builds of an app which uses my old branch, so I've added support for MinGW on current sources. And now I see this PR, so I guess we can work together to give this thing enough momentum.

See my latest branch: https://github.com/tehKaiN/tangram-es/tree/windows-v4 - builds with GCC7.3 dwarf2-posix. It's very barebones atm (lacks readme changes and font substitution handling) but I guess we can work together to add support for MSVC as well as MinGW. I've reviewed your code a bit. Notable points of interest in my fork in contrast to yours:

• very little changes to rest of code
• in url.c, support for windows paths by normalizing backslashes to slashes and support for disk letters
• initial support for default fonts (instead of predefined list in your branch) by querying your registry for installed ones - still needs to do using freetype or anything else to sieve through desired font characteristics and sort them by partial match
• I've abstracted the self-pipe thingy from urlclient to work using pipe on linux, and conjoined sockets on windows
• each change is thoroughly commented

stuff which needs more work/insights in your branch:

• there are lots of deletions and renames in cmake and c files - at this point I dunno if you've copypasted from earlier sources or is there a reasoning behind it since I see little to no in-code comments about the changes. The CMake stuff needs ifs since removed code is probably needed on other platforms
• I guess the patch can be submitted directly as a pull request in respective repo https://github.com/tangrams/geojson-vt-cpp

Would you be interested in rebasing your changes on top of mine? I think my latest branch is as little destructive to current code as possible. This way we can still have self-pipe etc. with only those butcherings which are needed to make it work with MSVC.

[sopyer]

@tehKaiN as I mentioned in PR its mix of your changes(I suspect outdated) and @mathisloge. I'll check your v4 branch later, it looks urlClient.cpp is a problem for me. As I said I can run application, but got 403 error. Also yes code still needs a lot of ifdefs and clean ups, and at the moment its PoC. But its first time I can run application, after few attempts.

[sopyer]

Also please note that I'll be using rebases for my changes, so please be aware of history rewrites.

[tehKaiN]

The 403 thingy can be related to how curl is built - are you building it using openssl or winssl?

[sopyer]

I use vcpkg for curl, no idea how its build, but so far I noticed url issue at least, still need to fix that first.

[mathisloge]

when consuming curl with the feature ssl, then curl will be build with winssl on windows and windows-mingw.
openssl will be used on linux and sectransp on osx.

you could force openssl when consuming curl with openssl feature instead of ssl

[sopyer]

Reduced a bit commit changes and was able to fetch urls now, though currently code crashes:

Debug Assertion Failed!

Program: D:\dev\gitrepo\tangram-es\out\build\x64-Debug\tangram.exe
File: C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.26.28801\include\vector
Line: 109

Expression: cannot seek vector iterator before begin

I first will make code work and later will check how we can merge branches. Currently I manually integrated changes of interest for me.

[matteblair]

Have you tried applying the change in #2271 to fix the 403 error?

[sopyer]

@matteblair yes it seems to be included with new urlClient.cpp/h from https://github.com/tehKaiN/tangram-es/tree/windows-v4

[sopyer]

Small update

I'll submit stops.cpp fix as separate PR later

[matteblair]

With the suggested patch to geojson-vt-cpp I was able to build and run in Visual Studio - great work to everyone who helped with this!

There is one failing test case now, a result of the URL parser being designed with no regard at all for Windows file paths. I think the change to the URL parser here is reasonable. Strictly speaking, it goes against the RFC that the logic is based on, but it would only cause a practical issue if one constructed a URL with a single-character scheme (I'm not aware of any that exist in real life). I recommend modifying the URLs in the test cases to use a scheme value like scheme instead of a single letter like g. I would also recommend adding test cases for Windows file paths, to make sure that absolute and relative paths with drive letters are resolved correctly. This might be a good change to merge incrementally, in fact.

Of course, reducing #ifdef branching is always desirable. Some of it will be unavoidable, but UrlClient might be possible to clean up even more.

Finally, it will be important to have automated builds of the Windows target to make sure that errors aren't introduced accidentally. Luckily, CircleCI has recently added support for running builds on Windows: https://circleci.com/docs/2.0/hello-world-windows/?section=executors-and-images I don't mind doing the configuration for this myself, since I am already familiar with the tools.

[sopyer]

There are still few other outstanding problems except for urlTests.

• TextWrapper tests crashing on Windows, and related probably issues with harfbuzz, icu
• select error at start which is most likely race condition. @tehKaiN @matteblair any reasons we use manual select instead of relying entirely on curl?
• compilation warnings, specifically for linking pthread

Can take care of URL tests and linker warnings, but not sure what to do with urlClient.cpp.

[tehKaiN]

select error at start which is most likely race condition. @tehKaiN @matteblair any reasons we use manual select instead of relying entirely on curl?

dunno really, I've just kept the thing as-is, I think it's needed for the self-pipe trick, I think the person who implemented it in the first place could give some more insights. Blame shows @hjanetzek and the commit comment suggests that this was needed in transitioning to curl API responsible fetching more urls in single thread.

[tehKaiN]

Now I see that the initial select() error is indeed caused by a race condition - the urlclient thread was set up earlier than the self-pipe was initialized. The self-pipe sockets had sometimes the -1 values when thread was looping earlier than the init has completed. See the latest commit on my v4 branch.

Why exactly do you have icu/harfbuzz disabled in your build? On MinGW it builds and links just fine - apparently works too.
Also, if you're doing something MSVC-specific, the if(WIN32) won't suffice since this gets enabled also on MinGW - you need to do if(WIN32 AND NOT MINGW) or if(MSVC), I think.

[tehKaiN]

I've managed to clean up the WINVER defines - this needs the change from my PR on harfbuzz-icu-freetype, but eliminates the warnings caused by redefinition of WINVER by GLFW. See my branch as usual.

This is probably more needed for MinGW since it targets winxp by default - the more recent MSVC are prob'ly targetting win7 by default.

[sopyer]

Fixed code a bit - url tests and urlClient.cpp race condition(thanks @tehKaiN).

Work left as I understand:

• pthread linker warnings
• fix TextWrapper tests
• update gitignore
• update README
• Add url tests for windows paths
• Integrate remaining MinGW code
• Best way to integrate geojson-vt-cpp patch
• CI

We can convert some of those points into issues. Let me know what you think.

[mathisloge]

do you plan on adding vcpkg.json in general?
If so, we can replace most of the dependencies under core/deps to vcpkg ports. For the geojson-vt-cpp patch we could add a overlay port and apply the patch on the go.

I don't know how the linkage for android and ios works, but i know that vcpkg can be build for ios/android.

[sopyer]

@mathisloge I got too many errors when trying to integrate geojson-vt-cpp from vcpkg. Just checked that all dependencies are properly forked so I just submit PR there. I think I just overcomplicated things a bit. In general my goal was to use as few vcpkg dependencies as possible in order to minimize changes.

[mathisloge]

yes, that is also all good. i think if it is basically wanted by the maintainers, i would also open an extra pr for it

[sopyer]

tangrams/geojson-vt-cpp#3 - PR for geojson-vt-cpp

[matteblair]

I am open to the idea of replacing some dependencies with a package manager! Personally, I don't have much experience with vcpkg so I need to spend some time investigating how it impacts the developer experience on platforms like Android and iOS.

For now, since adding a package manager isn't the main purpose of this PR, it seems best to do whatever causes the least disruption.

[mathisloge]

@matteblair #2274 have tested this branch with most of the dependencies replaced by vcpkg. But the results are looking kinda strange.

[sopyer]

Windows paths PR - #2276

[sopyer]

I think this should be minimalistic windows platform support. Remaining 4 bullet points can be converted to issues and addressed separately. List of PRs to submit before this one go in:

1. Added support for windows paths #2276
2. Fix windows compilation geojson-vt-cpp#3
3. Fix Windows debug assertion: "cannot seek vector iterator before begin" #2273

[tehKaiN]

I've reviewed your code a bit. The stuff pointed out makes diffs a bit messy - are those changes really needed?

[tehKaiN]

I've checked now for potential addition of MinGW stuff. I think it's better for submodules/dependencies to check for MSVC because vcpkg is more default for MS compilers - MinGW ppl tend to build their own packages by hand or use submodules where possible.

[mathisloge]

just out of curiosity @sopyer why are you force pushing the commits instead of "normal" commit / push?

[sopyer]

@mathisloge just old habit from using repo + gerrit - I prefer rebase workflow due to cleaner history.

[sopyer]

#2277 - GL_CHECK PR

[matteblair]

@sopyer I also prefer the history of main to remain clean, so I have this repo configured to squash PRs into a single commit when merged. So in the future, there's no need to squash commits within the PR itself. In fact, it can be helpful to see separate commits on an in-progress PR. (I should probably add this to the contributing guidelines.)

[tehKaiN]

Yeah, having history is better to navigate through the most recent changes. But we digress. When can we expect the merge? I'll add MinGW support after merge as a separate PR.

[matteblair]

The soonest I could review and merge this (and the related PRs) would be this weekend. Unfortunately my regular job keeps me busy on weekdays. Thanks for your patience!

[sopyer]

@matteblair I have updated PR with new pointer to geojson-vt-cpp. I think its ready to be merged in.

[matteblair]

Ok - thanks for your patience! I think we're good to merge this now. I have some minor adjustments in mind, but I will create separate PRs for those after merging. CI is working, aside from an intermittent issue with CircleCI usage limits: #2279

Thanks again for the work to implement this and the feedback on changes!