From 0bbb9be629ce35c11e4bf4a5180810ae2b16e5b4 Mon Sep 17 00:00:00 2001 From: dec05eba Date: Mon, 24 Sep 2018 15:57:53 +0200 Subject: Fix TODOs, mainly escaping strings for ninja --- backend/ninja/Ninja.cpp | 121 ++++++++++++++++++++++++++++++------------------ 1 file changed, 77 insertions(+), 44 deletions(-) (limited to 'backend/ninja/Ninja.cpp') 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 &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 Ninja::getLinkerFlags(const SibsConfig &config, LinkerFlagCallbackFunc staticLinkerFlagCallbackFunc, LinkerFlagCallbackFunc dynamicLinkerFlagCallback, GlobalIncludeDirCallbackFunc globalIncludeDirCallback, CflagsCallbackFunc cflagsCallbackFunc) const { const vector &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 globalLibDependencies; -#if OS_FAMILY == OS_FAMILY_POSIX vector 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 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 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 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; -- cgit v1.2.3