Fix TODOs, mainly escaping strings for ninja

2 files changed, 77 insertions, 45 deletions

diff --git a/backend/BackendUtils.cpp b/backend/BackendUtils.cpp
index 3c9dd71..4e1a0e8 100644
--- a/backend/BackendUtils.cpp
+++ b/backend/BackendUtils.cpp
@@ -88,7 +88,6 @@ namespace backend
             }
             else
             {
-                // TODO: Verify what happens if test has test sub directory
                 if (!sibsConfig.getTestPath().empty() && isPathSubPathOf(pathNative.c_str(), sibsConfig.getTestPath()))
                 {
                     string filePathUtf8 = toUtf8(pathNative.c_str());
diff --git a/backend/ninja/Ninja.cpp b/backend/ninja/Ninja.cpp
index 0b7dbe5..64f8f53 100644
--- a/backend/ninja/Ninja.cpp
+++ b/backend/ninja/Ninja.cpp
@@ -18,6 +18,26 @@ using namespace sibs;
 
 namespace backend
 {
+    static std::string escape(const std::string &str)
+    {
+        std::string result;
+        result.reserve(str.size());
+        for(char c : str)
+        {
+            if(c == '\n')
+                result += "$\n";
+            else if(c == '$')
+                result += "$$";
+            else if(c == ' ')
+                result += "$ ";
+            else if(c == ':')
+                result += "$:";
+            else
+                result += c;
+        }
+        return result;
+    }
+
     static string join(const vector<string> &list, const char *joinStr)
     {
         if(list.empty()) return "";
@@ -157,7 +177,10 @@ namespace backend
             }
             case Compiler::MSVC:
             {
-                // TODO: Is it possible to specify c version in msvc?
+                // It's not possible to specify C version in MSVC, programmer will have to instead be careful to not use features that are outside
+                // the desired version features...
+                // MSVC specific extensions can be disabled by /Za /Ze but we dont want to do that, code could have check if compiler is MSVC
+                // before using msvc extensions
                 return {};
             }
         }
@@ -183,7 +206,6 @@ namespace backend
             {
                 switch(cppVersion)
                 {
-                    // Use /Za flag?
                     case CPPVersion::CPP11: return { ninja::NinjaArg("/std=c++11") };
                     case CPPVersion::CPP14: return { ninja::NinjaArg("/std=c++14") };
                     case CPPVersion::CPP17: return { ninja::NinjaArg("/std=c++17") };
@@ -275,8 +297,6 @@ namespace backend
         return false;
     }
 
-    // TODO: First check if pkg-config is installed. If it's not, only check dependencies that exists in the dependencies sub directory.
-    // If pkg-config is installed and dependency is not installed, check in dependencies sub directory.
     Result<bool> Ninja::getLinkerFlags(const SibsConfig &config, LinkerFlagCallbackFunc staticLinkerFlagCallbackFunc, LinkerFlagCallbackFunc dynamicLinkerFlagCallback, GlobalIncludeDirCallbackFunc globalIncludeDirCallback, CflagsCallbackFunc cflagsCallbackFunc) const
     {
         const vector<PackageListDependency*> &packageListDependencies = config.getPackageListDependencies();
@@ -290,8 +310,8 @@ namespace backend
         if(createGlobalLibDirResult.isErr())
             return createGlobalLibDirResult;
 
+        // If pkg-config is not available then it will be ignored and we check if package instead exists as a global lib (on github etc)
         vector<PackageListDependency*> globalLibDependencies;
-#if OS_FAMILY == OS_FAMILY_POSIX
         vector<PackageListDependency*> pkgConfigDependencies;
         for(PackageListDependency *dependency : packageListDependencies)
         {
@@ -325,12 +345,6 @@ namespace backend
             if(!pkgConfigFlag.cflags.empty())
                 cflagsCallbackFunc(pkgConfigFlag.cflags);
         }
-#else
-		for (PackageListDependency *pkgConfigDependency : packageListDependencies)
-		{
-			globalLibDependencies.push_back(pkgConfigDependency);
-		}
-#endif
 
         return GlobalLib::getLibs(globalLibDependencies, config, globalLibDir, staticLinkerFlagCallbackFunc, dynamicLinkerFlagCallback, globalIncludeDirCallback);
     }
@@ -523,16 +537,24 @@ namespace backend
             char c = cLibraries[i];
             if(c == '\'' || c == '"')
             {
-                // TODO: Handle quote escaping
+                bool escapeQuote = false;
                 char quoteSymbol = c;
                 ++i;
                 size_t libraryIndexStart = i;
-                while(i < cLibraries.size() && cLibraries[i] != quoteSymbol)
+                while(i < cLibraries.size())
                 {
+                    if(cLibraries[i] == '\\')
+                        escapeQuote = !escapeQuote;
+                    else if(cLibraries[i] == quoteSymbol && !escapeQuote)
+                        break;
                     ++i;
                 }
                 size_t libraryIndexEnd = i;
                 ++i;
+
+                // This actually means the library path is malformed, but we can fix it here
+                if(escapeQuote)
+                    libraryIndexEnd--;
                 
                 if(strncmp(&cLibraries[libraryIndexStart], "-l", 2) == 0)
                     libraryIndexStart += 2;
@@ -545,15 +567,23 @@ namespace backend
             }
             else if(c != ' ')
             {
-                // TODO: Handle space escaping
+                bool escapeSpace = false;
                 size_t libraryIndexStart = i;
-                while(i < cLibraries.size() && cLibraries[i] != ' ')
+                while(i < cLibraries.size() && !escapeSpace && cLibraries[i] != ' ')
                 {
+                    if(cLibraries[i] == '\\')
+                        escapeSpace = !escapeSpace;
+                    else if(cLibraries[i] == ' ' && !escapeSpace)
+                        break;
                     ++i;
                 }
                 size_t libraryIndexEnd = i;
                 ++i;
 
+                // This actually means the library path is malformed, but we can fix it here
+                if(escapeSpace)
+                    libraryIndexEnd--;
+
                 if(strncmp(&cLibraries[libraryIndexStart], "-l", 2) == 0)
                     libraryIndexStart += 2;
                 else if(strncmp(&cLibraries[libraryIndexStart], "-pthread", libraryIndexEnd - libraryIndexStart) == 0)
@@ -597,7 +627,7 @@ namespace backend
         string result;
         for(const string &objectFile : objectFiles)
         {
-            result += " --object \"" + objectFile + "\"";
+            result += " --object " + escape(objectFile);
         }
         return result;
     }
@@ -751,11 +781,6 @@ namespace backend
             }
         }
 
-        // TODO: Verify ccache is installed
-        // -Werror
-        // TODO: Find equivalent -MMD -MP for other compilers than gcc. MMD is used to create "dependency files" -> if headers are modified then source files will be recompiled
-        // when compiling next time...
-
         // TODO: Verify compilers for language are installed when attempting to build a project that has source files of those types
 
         vector<ninja::NinjaArg> compileCCommand;
@@ -843,10 +868,10 @@ namespace backend
             case Compiler::MSVC:
             {
                 compileCCommand.insert(compileCCommand.end(), { 
-                    ninja::NinjaArg("cl.exe"),
-                    ninja::NinjaArg("/c"), 
-                    ninja::NinjaArg("$in"), 
-                    ninja::NinjaArg("/Fo$out")
+                    ninja::NinjaArg::createRaw("cl.exe /showIncludes"),
+                    ninja::NinjaArg::createRaw("/c"), 
+                    ninja::NinjaArg::createRaw("$in"), 
+                    ninja::NinjaArg::createRaw("/Fo$out")
                 });
 
                 compileCCommand.insert(compileCCommand.end(), cflags.begin(), cflags.end());
@@ -888,10 +913,23 @@ namespace backend
         compileCppCommand.insert(compileCppCommand.end(), cppLanguageVersionFlags.begin(), cppLanguageVersionFlags.end());
 
         ninja::NinjaRule *compileCRule = ninjaBuildFile.createRule("compile_c", compileCCommand);
-        compileCRule->depFile = "$out.d";
-
         ninja::NinjaRule *compileCppRule = ninjaBuildFile.createRule("compile_cpp", compileCppCommand);
-        compileCppRule->depFile = "$out.d";
+        
+        switch(config.getCompiler())
+        {
+            case Compiler::GCC:
+            {
+                compileCRule->depFile = "$out.d";
+                compileCppRule->depFile = "$out.d";
+                break;
+            }
+            case Compiler::MSVC:
+            {
+                compileCRule->deps = "msvc";
+                compileCppRule->deps = "msvc";
+                break;
+            }
+        }
 
         // TODO: Specify -mconsole or -mwindows for windows.
         // TODO: Convert sibs defines to const variables in a zig file that other zig files can include (like a config file).
@@ -962,12 +1000,9 @@ namespace backend
         {
             if(sourceFile.language == sibs::Language::ZIG)
             {
-                // TODO: Remove this and use NinjaBuilds to get object names
                 string objectName = config.getPackageName() + "@exe/" + sourceFile.filepath;
                 objectName += getObjectFileExtension(config.getCompiler());
-                objectNames.emplace_back(objectName);
 
-                // TODO: Optimize this by checking if directory has already been created with a hash map
                 int filenameLength = getFilenameLength(sourceFile.filepath);
                 string zigSourceDirRelative = sourceFile.filepath.substr(0, sourceFile.filepath.size() - filenameLength);
                 FileString zigHeaderFileDir = toFileString(generatedZigHeaderDirUtf8 + '/' + zigSourceDirRelative);
@@ -983,6 +1018,8 @@ namespace backend
                 else
                     ninjaBuild = compileZigRule->build("../../" + sourceFile.filepath, objectName, { zigHeaderFileValue });
                 zigBuilds.push_back(ninjaBuild);
+
+                objectNames.emplace_back(move(objectName));
             }
             else
                 onlyZigFiles = false;
@@ -995,7 +1032,6 @@ namespace backend
             {
                 case sibs::Language::C:
                 {
-                    // TODO: Verify what happens if object name contains colon or space
                     objectName += config.getPackageName() + "@exe/" + sourceFile.filepath;
                     objectName += getObjectFileExtension(config.getCompiler());
                     compileCRule->build("../../" + sourceFile.filepath, objectName, {}, zigBuilds);
@@ -1019,7 +1055,9 @@ namespace backend
                     assert(false);
                     break;
             }
-            objectNames.emplace_back(objectName);
+
+            if(!objectName.empty())
+                objectNames.emplace_back(move(objectName));
         }
 
         // TODO: For now zig projects (zig object files) are built with c/c++ compiler,
@@ -1057,7 +1095,7 @@ namespace backend
                         buildExeArgs.insert(buildExeArgs.end(), zigLibraryFlags.begin(), zigLibraryFlags.end());
 
                         ninja::NinjaRule *buildExeRule = ninjaBuildFile.createRule("build_exec", buildExeArgs);
-                        buildExeRule->build(join(objectNames, " "), config.getPackageName(), { zigObjectArgsValue });
+                        buildExeRule->build(objectNames, config.getPackageName(), { zigObjectArgsValue });
                     }
                     else
                     {
@@ -1115,7 +1153,7 @@ namespace backend
                             buildExeArgs.push_back(ninja::NinjaArg::createRaw(allLinkerFlags));
 
                         ninja::NinjaRule *buildExeRule = ninjaBuildFile.createRule("build_exec", buildExeArgs);
-                        buildExeRule->build(join(objectNames, " "), config.getPackageName(), {});
+                        buildExeRule->build(objectNames, config.getPackageName(), {});
                     }
                     
                     projectGeneratedBinary += config.getPackageName();
@@ -1153,12 +1191,8 @@ namespace backend
                         });
                         buildStaticArgs.insert(buildStaticArgs.end(), commonZigArgs.begin(), commonZigArgs.end());
 
-                        // TODO: Verify if this is needed. It's not needed for c/c++ files...
-                        //vector<ninja::NinjaArg> zigLibraryFlags = convertCLibrariesToZigLibraries(config.getCompiler(), allLinkerFlags);
-                        //buildStaticArgs.insert(buildStaticArgs.end(), zigLibraryFlags.begin(), zigLibraryFlags.end());
-
                         ninja::NinjaRule *buildStaticRule = ninjaBuildFile.createRule("build_static", buildStaticArgs);
-                        buildStaticRule->build(join(objectNames, " "), generatedFile, { zigObjectArgsValue });
+                        buildStaticRule->build(objectNames, generatedFile, { zigObjectArgsValue });
                     }
                     else
                     {
@@ -1186,7 +1220,7 @@ namespace backend
                         }
 
                         ninja::NinjaRule *buildStaticRule = ninjaBuildFile.createRule("build_static", buildStaticArgs);
-                        buildStaticRule->build(join(objectNames, " "), generatedFile, {});
+                        buildStaticRule->build(objectNames, generatedFile, {});
                     }
                     
                     projectGeneratedBinary += generatedFile;
@@ -1228,7 +1262,7 @@ namespace backend
                         buildDynamicArgs.insert(buildDynamicArgs.end(), zigLibraryFlags.begin(), zigLibraryFlags.end());
 
                         ninja::NinjaRule *buildDynamicRule = ninjaBuildFile.createRule("build_dynamic", buildDynamicArgs);
-                        buildDynamicRule->build(join(objectNames, " "), generatedFile, { zigObjectArgsValue });
+                        buildDynamicRule->build(objectNames, generatedFile, { zigObjectArgsValue });
                     }
                     else
                     {
@@ -1286,7 +1320,7 @@ namespace backend
                             buildDynamicArgs.push_back(ninja::NinjaArg::createRaw(allLinkerFlags));
 
                         ninja::NinjaRule *buildDynamicRule = ninjaBuildFile.createRule("build_dynamic", buildDynamicArgs);
-                        buildDynamicRule->build(join(objectNames, " "), generatedFile, {});
+                        buildDynamicRule->build(objectNames, generatedFile, {});
                     }
                     
                     projectGeneratedBinary += generatedFile;
@@ -1455,7 +1489,6 @@ namespace backend
 
     Result<bool> Ninja::compile(const _tinydir_char_t *buildFilePath)
     {
-        // TODO: Verify `script` is installed. We need `script` to get colored output
 #if OS_FAMILY == OS_FAMILY_POSIX
         FileString command = TINYDIR_STRING("script -eqc 'ninja -C \"");
         command += buildFilePath;