Clean up for CMake scripts and Windows platform (#2287)

* Add CMake option to use vcpkg

* Add Url::fromWindowsFilePath and remove platform-dependent behavior in Url

* Tweak TANGRAM_USE_VCPKG logic

* include CmakeDependentOption module

* Add YAML::loadNoCopy to YamlUtil

* Fix ICU linkage when using vcpkg on Linux

* Use pinned version of vcpkg for Windows CI

* Fix harfbuzz linkage when not using vcpkg

* vcpkg harfbuzz needs coretext feature on macos

* vcpkg harfbuzz doesn't need graphite2 feature

* Bust vcpkg cache on windows CI

.github/workflows/windows.yml

@@ -9,15 +9,21 @@ jobs:
         uses: actions/checkout@v2
         with:
           submodules: true
+      - name: "Checkout vcpkg"
+        uses: actions/checkout@v2
+        with:
+          repository: microsoft/vcpkg
+          ref: 2021.05.12
+          path: vcpkg
       - name: "Cache vcpkg binaries"
         uses: actions/cache@v2
         with:
           path: ~\AppData\Local\vcpkg\archives
-          key: vcpkg-v1-${{ hashFiles('vcpkg.json') }}
+          key: vcpkg-v2-${{ hashFiles('vcpkg.json') }}
           restore-keys: vcpkg-v1-
       - name: "Configure CMake"
         run: cmake -S . -B .\build -G "Visual Studio 16 2019" \
-          -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" \
+          -DCMAKE_TOOLCHAIN_FILE=".\vcpkg\scripts\buildsystems\vcpkg.cmake" \
           -DCMAKE_BUILD_TYPE=Debug
           -DTANGRAM_BUILD_TESTS=1
       - name: "Run Build"

CMakeLists.txt

@@ -16,10 +16,16 @@ option(TANGRAM_BUNDLE_TESTS "Compile all tests into a single binary" ON)
 option(TANGRAM_BUILD_BENCHMARKS "Build benchmarks" OFF)
 option(TANGRAM_DEV_MODE "For developers only: Don't omit the frame pointer" OFF)
 
+include(CMakeDependentOption)
+# Allow choosing whether to use vcpkg for dependencies when vcpkg toolchain is present, otherwise force off.
+CMAKE_DEPENDENT_OPTION(TANGRAM_USE_VCPKG "Use vcpkg for 3rd-party dependencies" ON "VCPKG_TOOLCHAIN" OFF)
+message(STATUS "Use vcpkg: ${TANGRAM_USE_VCPKG}")
+
+
 message(STATUS "Build type configuration: ${CMAKE_BUILD_TYPE}")
 
 # Check that submodules are present.
-if(NOT MSVC AND NOT EXISTS "${PROJECT_SOURCE_DIR}/core/deps/harfbuzz-icu-freetype/harfbuzz/README")
+if(NOT EXISTS "${PROJECT_SOURCE_DIR}/core/deps/isect2d/README.md")
   message(SEND_ERROR "Missing submodules - Please run:\n 'git submodule update --init'")
   return()
 endif()

core/CMakeLists.txt

@@ -2,7 +2,8 @@ project(tangram-core)
 
 # Build core library dependencies.
 find_package(ZLIB REQUIRED)
-if (MSVC)
+
+if (TANGRAM_USE_VCPKG)
   find_package(yaml-cpp CONFIG REQUIRED)
 endif()
 

core/deps/CMakeLists.txt

@@ -13,13 +13,12 @@ target_compile_definitions(glm INTERFACE GLM_FORCE_CTOR_INIT)
 
 ## yaml-cpp ##
 ##############
-if (NOT MSVC)
+if(NOT TANGRAM_USE_VCPKG)
   set(YAML_CPP_BUILD_TESTS OFF CACHE BOOL "")
   set(YAML_CPP_BUILD_TOOLS OFF CACHE BOOL "")
   set(YAML_CPP_BUILD_CONTRIB OFF CACHE BOOL "")
   set(YAML_CPP_INSTALL OFF CACHE BOOL "")
   add_subdirectory(yaml-cpp)
-
   target_include_directories(yaml-cpp PUBLIC yaml-cpp/include)
 endif()
 
@@ -46,26 +45,20 @@ target_include_directories(miniz
   ${CMAKE_CURRENT_SOURCE_DIR}/miniz)
 
 
-
+## Harfbuzz - ICU-Common - Freetype2 ##
+#######################################
 if(NOT TANGRAM_USE_SYSTEM_FONT_LIBS)
-  ## Harfbuzz - ICU-Common - Freetype2 ##
-  #######################################
-  if(MSVC)
-    #find_package(ICU REQUIRED COMPONENTS common)
+  if(TANGRAM_USE_VCPKG)
+    find_package(ICU REQUIRED COMPONENTS data)
     find_package(harfbuzz CONFIG REQUIRED)
-    set(HARFBUZZ_LIB_NAME harfbuzz::harfbuzz)
+    set(ALFONS_LIBS harfbuzz::harfbuzz ICU::data)
   else()
     set(HARFBUZZ_BUILD_ICU ON CACHE BOOL "Enable building of ICU")
     add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/harfbuzz-icu-freetype)
-
-    message(STATUS "harfbuzz" ${HARFBUZZ_LIBRARIES})
-    set(HARFBUZZ_LIB_NAME harfbuzz ${HARFBUZZ_LIBRARIES})
+    set(ALFONS_LIBS harfbuzz ${HARFBUZZ_LIBRARIES})
   endif()
 
-  set(ALFONS_DEPS_LIBRARIES
-    ${ALFONS_DEPS_LIBRARIES}
-    ${HARFBUZZ_LIB_NAME}
-    CACHE INTERNAL "alfons-libs" FORCE)
+  set(ALFONS_DEPS_LIBRARIES ${ALFONS_LIBS} CACHE INTERNAL "alfons-libs" FORCE)
 endif()
 
 ## alfons ##

core/include/tangram/util/url.h

@@ -123,6 +123,9 @@ class Url {
     // replaced with the corresponding UTF-8 characters.
     static std::string unEscapeReservedCharacters(const std::string& in);
 
+    // Return a URL representing the given Windows file path.
+    static Url fromWindowsFilePath(const std::string& windowsPath);
+
 private:
 
     // buffer contains the actual text of the URL.

core/src/scene/importer.cpp

@@ -3,6 +3,7 @@
 #include "log.h"
 #include "platform.h"
 #include "util/asyncWorker.h"
+#include "util/yamlUtil.h"
 #include "util/zipArchive.h"
 
 #include <algorithm>
@@ -188,11 +189,7 @@ void Importer::addSceneYaml(const Url& sceneUrl, const char* sceneYaml, size_t l
     auto& sceneNode = m_sceneNodes[sceneUrl];
 
     try {
-#ifdef _MSC_VER
-        sceneNode.yaml = YAML::Load(sceneYaml);
-#else
-        sceneNode.yaml = YAML::Load(sceneYaml, length);
-#endif
+        sceneNode.yaml = YamlUtil::loadNoCopy(sceneYaml, length);
     } catch (const YAML::ParserException& e) {
         LOGE("Parsing scene config '%s'", e.what());
         return;

core/src/util/url.cpp

@@ -321,11 +321,6 @@ std::string Url::removeDotSegmentsFromString(std::string path) {
 }
 
 void Url::parse() {
-#ifdef TANGRAM_WINDOWS
-    // Normalize Windows paths. Chromium also does this.
-    std::replace(buffer.begin(), buffer.end(), '\\', '/');
-#endif
-
     // The parsing process roughly follows https://tools.ietf.org/html/rfc1808#section-2.4
 
     size_t start = 0;
@@ -604,4 +599,23 @@ std::string Url::unEscapeReservedCharacters(const std::string& in) {
     return out;
 }
 
+Url Url::fromWindowsFilePath(const std::string& windowsPath) {
+    // https://docs.microsoft.com/en-us/archive/blogs/ie/file-uris-in-windows
+    // Strictly speaking this should also %-escape any reserved characters in the Windows path segments,
+    // but in practice this doesn't seem to change file access behavior. 
+    std::string urlString;
+    urlString.reserve(8 + windowsPath.size());
+    urlString += "file:///";
+    for (auto it = windowsPath.begin(), end = windowsPath.end(); it != end; ++it) {
+        // Normalize backward slashes to forward slashes.
+        if (*it == '\\') {
+            urlString += '/';
+        } else {
+            urlString += *it;
+        }
+    }
+
+    return Url(urlString);
+}
+
 } // namespace Tangram

core/src/util/yamlUtil.cpp

@@ -11,6 +11,17 @@
 namespace Tangram {
 namespace YamlUtil {
 
+struct memstream : public std::istream, public std::streambuf {
+    memstream(const char* begin, const char* end): std::istream(this)  {
+        setg(const_cast<char *>(begin), const_cast<char *>(begin), const_cast<char *>(end));
+    }
+};
+
+YAML::Node loadNoCopy(const char* input, size_t length) {
+    memstream stream(input, input + length);
+    return YAML::Load(stream);
+}
+
 glm::vec4 getColorAsVec4(const YAML::Node& node) {
     double val;
     if (getDouble(node, val, false)) {

core/src/util/yamlUtil.h

@@ -9,6 +9,8 @@
 namespace Tangram {
 namespace YamlUtil {
 
+YAML::Node loadNoCopy(const char* input, size_t length);
+
 bool getInt(const YAML::Node& node, int& result, bool allowTrailingJunk = false);
 
 int getIntOrDefault(const YAML::Node& node, int defaultValue, bool allowTrailingJunk = false);

platforms/windows/src/main.cpp

@@ -18,10 +18,10 @@ int main(int argc, char* argv[]) {
     GlfwApp::parseArgs(argc, argv);
 
     // Resolve the input path against the current directory.
-    Url baseUrl("file://");
+    Url baseUrl("file:///");
     char pathBuffer[PATH_MAX] = {0};
     if (getcwd(pathBuffer, PATH_MAX) != nullptr) {
-        baseUrl = baseUrl.resolve(Url("/" + std::string(pathBuffer) + "/"));
+        baseUrl = Url::fromWindowsFilePath(std::string(pathBuffer) + "/");
     }
 
     LOG("Base URL: %s", baseUrl.string().c_str());

vcpkg.json

@@ -14,12 +14,20 @@
         "yaml-cpp",
         "glfw3",
         "benchmark",
+        {
+            "name": "harfbuzz",
+            "features": [
+                "icu"
+            ],
+            "platform": "!osx"
+        },
         {
             "name": "harfbuzz",
             "features": [
                 "icu",
-                "graphite2"
-            ]
+                "coretext"
+            ],
+            "platform": "osx"
         },
         {
             "name": "imgui",