fixup font registration code ensuring invalid fonts will warn but not throw and register_fonts will only return success if > one font is registered and none have failed

Makefile

@@ -27,4 +27,7 @@ pep8:
 	@pep8 -r --select=W293 -q --filename=*.py `pwd`/tests/ | xargs gsed -i 's/^[ \r\t]*$//'
 	@pep8 -r --select=W391 -q --filename=*.py `pwd`/tests/ | xargs gsed -i -e :a -e '/^\n*$/{$d;N;ba' -e '}'
 
+grind:
+	@valgrind --leak-check=full tests/cpp_tests/font_registration_test
+
 .PHONY: clean reset uninstall test install

SConstruct

@@ -1716,7 +1716,7 @@ if not HELP_REQUESTED:
     
     # build C++ tests
     # not ready for release
-    #SConscript('tests/cpp_tests/build.py')
+    SConscript('tests/cpp_tests/build.py')
     
     # not ready for release
     #if env['SVG_RENDERER']:

src/font_engine_freetype.cpp

@@ -71,11 +71,10 @@ bool freetype_engine::is_font_file(std::string const& file_name)
 
 bool freetype_engine::register_font(std::string const& file_name)
 {
-    if (!boost::filesystem::is_regular_file(file_name) || !is_font_file(file_name)) return false;
 #ifdef MAPNIK_THREADSAFE
     mutex::scoped_lock lock(mutex_);
 #endif
-    FT_Library library;
+    FT_Library library = 0;
     FT_Error error = FT_Init_FreeType(&library);
     if (error)
     {
@@ -84,6 +83,7 @@ bool freetype_engine::register_font(std::string const& file_name)
 
     FT_Face face = 0;
     int num_faces = 0;
+    bool success = false;
     // some font files have multiple fonts in a file
     // the count is in the 'root' face library[0]
     // see the FT_FaceRec in freetype.h
@@ -92,31 +92,37 @@ bool freetype_engine::register_font(std::string const& file_name)
         error = FT_New_Face (library,file_name.c_str(),i,&face);
         if (error)
         {
-            FT_Done_FreeType(library);
+            break;
-            return false;
         }
         // store num_faces locally, after FT_Done_Face it can not be accessed any more
         if (!num_faces)
             num_faces = face->num_faces;
         // some fonts can lack names, skip them
         // http://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_FaceRec
-        if (face->family_name && face->style_name) {
+        if (face->family_name && face->style_name)
+        {
+            success = true;
             std::string name = std::string(face->family_name) + " " + std::string(face->style_name);
             name2file_.insert(std::make_pair(name, std::make_pair(i,file_name)));
-            FT_Done_Face(face);
+        }
-            //FT_Done_FreeType(library);
+        else
-            //return true;
+        {
-        } else {
-            FT_Done_Face(face);
-            FT_Done_FreeType(library);
             std::ostringstream s;
-            s << "Error: unable to load invalid font file which lacks identifiable family and style name: '"
+            s << "Warning: unable to load font file '" << file_name << "' ";
-              << file_name << "'";
+            if (!face->family_name && !face->style_name)
-            throw std::runtime_error(s.str());
+                s << "which lacks both a family name and style name";
+            else if (face->family_name)
+                s << "which reports a family name of '" << std::string(face->family_name) << "' and lacks a style name";
+            else if (face->style_name)
+                s << "which reports a style name of '" << std::string(face->style_name) << "' and lacks a family name";
+            std::clog << s.str() << std::endl;
         }
     }
+    if (face)
+        FT_Done_Face(face);
+    if (library)
         FT_Done_FreeType(library);
-    return true;
+    return success;
 }
 
 bool freetype_engine::register_fonts(std::string const& dir, bool recurse)
@@ -130,26 +136,24 @@ bool freetype_engine::register_fonts(std::string const& dir, bool recurse)
         return mapnik::freetype_engine::register_font(dir);
 
     boost::filesystem::directory_iterator end_itr;
+    bool success = false;
     for (boost::filesystem::directory_iterator itr(dir); itr != end_itr; ++itr)
     {
+#if (BOOST_FILESYSTEM_VERSION == 3)
+        std::string const& file_name = itr->path().string();
+#else // v2
+        std::string const& file_name = itr->string();
+#endif
         if (boost::filesystem::is_directory(*itr) && recurse)
         {
-#if (BOOST_FILESYSTEM_VERSION == 3)
+            success = register_fonts(file_name, true);
-            if (!register_fonts(itr->path().string(), true)) return false;
-#else // v2
-            if (!register_fonts(itr->string(), true)) return false;
-#endif
         }
-        else
+        else if (boost::filesystem::is_regular_file(file_name) && is_font_file(file_name))
         {
-#if (BOOST_FILESYSTEM_VERSION == 3)
+            success = mapnik::freetype_engine::register_font(file_name);
-            mapnik::freetype_engine::register_font(itr->path().string());
-#else // v2
-            mapnik::freetype_engine::register_font(itr->string());
-#endif
         }
     }
-    return true;
+    return success;
 }
 
 

src/load_map.cpp

@@ -242,7 +242,13 @@ void map_parser::parse_map(Map & map, xml_node const& pt, std::string const& bas
             if (font_directory)
             {
                 extra_attr["font-directory"] = *font_directory;
-                freetype_engine::register_fonts(ensure_relative_to_xml(font_directory), false);
+                if (!freetype_engine::register_fonts(ensure_relative_to_xml(font_directory), false))
+                {
+                    if (strict_)
+                    {
+                        throw config_error(std::string("Failed to load fonts from: ") << *font_directory);
+                    }
+                }
             }
 
             optional<std::string> min_version_string = map_node.get_opt_attr<std::string>("minimum-version");

tests/cpp_tests/build.py

@@ -11,6 +11,8 @@ headers = env['CPPPATH']
 libraries =  copy(env['LIBMAPNIK_LIBS'])
 libraries.append('mapnik')
 
+test_env.Append(CXXFLAGS='-g')
+
 for cpp_test in glob.glob('*_test.cpp'):
     test_program = test_env.Program(cpp_test.replace('.cpp',''), [cpp_test], CPPPATH=headers, LIBS=libraries, LINKFLAGS=env['CUSTOM_LDFLAGS'])
     Depends(test_program, env.subst('../../src/%s' % env['MAPNIK_LIB_NAME']))

tests/cpp_tests/font_registration_test.cpp

@@ -6,8 +6,6 @@ using fs::path;
 namespace sys = boost::system;
 
 #include <boost/detail/lightweight_test.hpp>
-//#include <boost/bind.hpp>
-//#include <fstream>
 #include <iostream>
 #include <mapnik/font_engine_freetype.hpp>
 
@@ -39,14 +37,23 @@ int main( int, char*[] )
 
   // directories without fonts
   std::string src("src");
-  // a legitimate directory will return true even it is does not 
+  // an empty directory will not return true
-  // successfully register a font...
+  // we need to register at least one font and not fail on any
-  BOOST_TEST( mapnik::freetype_engine::register_fonts(src , true ) );
+  // to return true
-  face_names = mapnik::freetype_engine::face_names();
+  BOOST_TEST( mapnik::freetype_engine::register_font(src) == false );
-  BOOST_TEST( face_names.size() == 0 );
+  BOOST_TEST( mapnik::freetype_engine::register_fonts(src, true) == false );
-  std::clog << "number of registered fonts: " << face_names.size() << std::endl;
+  BOOST_TEST( mapnik::freetype_engine::face_names().size() == 0 );
 
-  // register unifont
+  // bogus, emtpy file that looks like font
+  BOOST_TEST( mapnik::freetype_engine::register_font("tests/data/fonts/fake.ttf") == false );
+  BOOST_TEST( mapnik::freetype_engine::register_fonts("tests/data/fonts/fake.ttf") == false );
+  BOOST_TEST( mapnik::freetype_engine::face_names().size() == 0 );
+
+  BOOST_TEST( mapnik::freetype_engine::register_font("tests/data/fonts/intentionally-broken.ttf") == false );
+  BOOST_TEST( mapnik::freetype_engine::register_fonts("tests/data/fonts/intentionally-broken.ttf") == false );
+  BOOST_TEST( mapnik::freetype_engine::face_names().size() == 0 );
+
+  // register unifont, since we know it sits in the root fonts/ dir
   BOOST_TEST( mapnik::freetype_engine::register_fonts(fontdir) );
   face_names = mapnik::freetype_engine::face_names();
   std::clog << "number of registered fonts: " << face_names.size() << std::endl;
@@ -71,7 +78,7 @@ int main( int, char*[] )
   BOOST_TEST( mapnik::freetype_engine::register_fonts(fontdir, true) );
   face_names = mapnik::freetype_engine::face_names();
   std::clog << "number of registered fonts: " << face_names.size() << std::endl;
-  BOOST_TEST( face_names.size() == 21 );
+  BOOST_TEST( face_names.size() == 22 );
 
   return ::boost::report_errors();
 }

tests/data/good_maps/rtl_text_map.xml

@@ -1,4 +1,6 @@
-<!-- nik2img.py -b 60 46 80 28 rtl_text_map.xml text_styles.png --><Map background-color="#eee" srs="+init=epsg:4326" minimum-version="0.7.2" font-directory="../fonts/">
+<Map background-color="#eee" srs="+init=epsg:4326" minimum-version="0.7.2" font-directory="../fonts/XB Zar.ttf">
+  
+<!-- nik2img.py -b 60 46 80 28 rtl_text_map.xml text_styles.png -->
 
   <Style name="custom_font">
     <Rule>