From 44f29a4291c9de6f8a1ff9097cd36576d987da14 Mon Sep 17 00:00:00 2001 From: Evan Wilde Date: Fri, 1 Dec 2023 18:23:16 -0800 Subject: Swift/Ninja: Fix multifile module compile commands Swift compile commands need to have all source files in the module specified in the compile command or LSP systems will report errors on missing types that are defined in other source files in the same module. Issue: #25491 --- Source/cmNinjaTargetGenerator.cxx | 98 +++++++++++++++++++----- Source/cmNinjaTargetGenerator.h | 6 ++ Tests/RunCMake/Swift/CompileCommands-check.cmake | 30 ++++++++ Tests/RunCMake/Swift/CompileCommands.cmake | 9 +++ Tests/RunCMake/Swift/RunCMakeTest.cmake | 5 ++ 5 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 Tests/RunCMake/Swift/CompileCommands-check.cmake create mode 100644 Tests/RunCMake/Swift/CompileCommands.cmake diff --git a/Source/cmNinjaTargetGenerator.cxx b/Source/cmNinjaTargetGenerator.cxx index 7ea479e..2ea893d 100644 --- a/Source/cmNinjaTargetGenerator.cxx +++ b/Source/cmNinjaTargetGenerator.cxx @@ -1916,7 +1916,6 @@ void cmNinjaTargetGenerator::WriteSwiftObjectBuildStatement( // indistinguishable from the old behavior. // // FIXME(#25490): Add response file support to Swift object build step - // FIXME(#25491): Include all files in module in compile_commands.json if (sources.empty()) { return; @@ -2027,15 +2026,7 @@ void cmNinjaTargetGenerator::WriteSwiftObjectBuildStatement( std::string const sourceFilePath = this->GetCompiledSourceNinjaPath(sf); objBuild.ExplicitDeps.push_back(sourceFilePath); - if (isSingleOutput) { - if (firstForConfig) { - this->ExportObjectCompileCommand( - language, sourceFilePath, objectDir, targetObjectFilename, - cmSystemTools::GetFilenamePath(targetObjectFilename), vars["FLAGS"], - vars["DEFINES"], vars["INCLUDES"], - /*compile pdb*/ "", /*target pdb*/ "", config, WithScanning::No); - } - } else { + if (!isSingleOutput) { // Object outputs std::string const objectFilepath = this->ConvertToNinjaPath(this->GetObjectFilePath(sf, config)); @@ -2045,16 +2036,6 @@ void cmNinjaTargetGenerator::WriteSwiftObjectBuildStatement( // Add OFM data this->EmitSwiftDependencyInfo(sf, config); - - // Emit compile commands - if (firstForConfig) { - this->ExportObjectCompileCommand( - language, sourceFilePath, objectDir, objectFilepath, - cmSystemTools::GetFilenamePath(objectFilepath), vars["FLAGS"], - vars["DEFINES"], vars["INCLUDES"], - /*compile pdb*/ "", - /*target pdb*/ "", config, WithScanning::No); - } } } @@ -2062,6 +2043,12 @@ void cmNinjaTargetGenerator::WriteSwiftObjectBuildStatement( this->GenerateSwiftOutputFileMap(config, vars["FLAGS"]); } + if (firstForConfig) { + this->ExportSwiftObjectCompileCommand( + sources, targetObjectFilename, vars["FLAGS"], vars["DEFINES"], + vars["INCLUDES"], config, isSingleOutput); + } + for (cmTargetDepend const& dep : this->GetGlobalGenerator()->GetTargetDirectDepends(&target)) { if (!dep->IsLanguageUsed("Swift", config)) { @@ -2326,6 +2313,77 @@ void cmNinjaTargetGenerator::ExportObjectCompileCommand( objectFileName); } +void cmNinjaTargetGenerator::ExportSwiftObjectCompileCommand( + std::vector const& moduleSourceFiles, + std::string const& moduleObjectFilename, std::string const& flags, + std::string const& defines, std::string const& includes, + std::string const& outputConfig, bool singleOutput) +{ + if (!this->GeneratorTarget->GetPropertyAsBool("EXPORT_COMPILE_COMMANDS")) { + return; + } + + auto escapeSourceFileName = [this](std::string srcFilename) -> std::string { + if (!cmSystemTools::FileIsFullPath(srcFilename)) { + srcFilename = + cmSystemTools::CollapseFullPath(srcFilename, + this->GetGlobalGenerator() + ->GetCMakeInstance() + ->GetHomeOutputDirectory()); + } + + return this->LocalGenerator->ConvertToOutputFormat( + srcFilename, cmOutputConverter::SHELL); + }; + + cmRulePlaceholderExpander::RuleVariables compileObjectVars; + compileObjectVars.Language = "Swift"; + compileObjectVars.Flags = flags.c_str(); + compileObjectVars.Defines = defines.c_str(); + compileObjectVars.Includes = includes.c_str(); + + // Build up the list of source files in the module + std::vector filenames; + filenames.reserve(moduleSourceFiles.size()); + for (cmSourceFile const* sf : moduleSourceFiles) { + filenames.emplace_back( + escapeSourceFileName(this->GetCompiledSourceNinjaPath(sf))); + } + // Note that `escapedSourceFilenames` must remain alive until the + // compileObjectVars is consumed or Source will be a dangling pointer. + std::string const escapedSourceFilenames = cmJoin(filenames, " "); + compileObjectVars.Source = escapedSourceFilenames.c_str(); + + std::string const& compileCommand = + this->Makefile->GetRequiredDefinition("CMAKE_Swift_COMPILE_OBJECT"); + cmList compileCmds(compileCommand); + + auto rulePlaceholderExpander = + this->GetLocalGenerator()->CreateRulePlaceholderExpander(); + + for (cmSourceFile const* sf : moduleSourceFiles) { + std::string const sourceFilename = this->GetCompiledSourceNinjaPath(sf); + std::string objectFilename = moduleObjectFilename; + + if (!singleOutput) { + // If it's not single-output, each source file gets a separate object + objectFilename = this->GetObjectFilePath(sf, outputConfig); + } + compileObjectVars.Objects = objectFilename.c_str(); + + for (std::string& cmd : compileCmds) { + rulePlaceholderExpander->ExpandRuleVariables(this->GetLocalGenerator(), + cmd, compileObjectVars); + } + + std::string commandLine = this->GetLocalGenerator()->BuildCommandLine( + compileCmds, outputConfig, outputConfig); + + this->GetGlobalGenerator()->AddCXXCompileCommand( + commandLine, sourceFilename, objectFilename); + } +} + void cmNinjaTargetGenerator::AdditionalCleanFiles(const std::string& config) { if (cmValue prop_value = diff --git a/Source/cmNinjaTargetGenerator.h b/Source/cmNinjaTargetGenerator.h index b55c460..f081117 100644 --- a/Source/cmNinjaTargetGenerator.h +++ b/Source/cmNinjaTargetGenerator.h @@ -195,6 +195,12 @@ protected: std::string const& targetCompilePdb, std::string const& targetPdb, std::string const& outputConfig, WithScanning withScanning); + void ExportSwiftObjectCompileCommand( + std::vector const& moduleSourceFiles, + std::string const& moduleObjectFilename, std::string const& flags, + std::string const& defines, std::string const& includes, + std::string const& outputConfig, bool singleOutput); + void AdditionalCleanFiles(const std::string& config); cmNinjaDeps GetObjects(const std::string& config) const; diff --git a/Tests/RunCMake/Swift/CompileCommands-check.cmake b/Tests/RunCMake/Swift/CompileCommands-check.cmake new file mode 100644 index 0000000..6450745 --- /dev/null +++ b/Tests/RunCMake/Swift/CompileCommands-check.cmake @@ -0,0 +1,30 @@ +if(NOT EXISTS "${RunCMake_TEST_BINARY_DIR}/compile_commands.json") + set(RunCMake_TEST_FAILED "compile_commands.json not generated") + return() +endif() + +# The compile command for both files should contain all Swift source files in +# the module +set(expected_compile_commands +[==[^\[ +{ + "directory": ".*(/Tests/RunCMake/Swift/CompileCommands-build|\\\\Tests\\\\RunCMake\\\\Swift\\\\CompileCommands-build)", + "command": ".*swiftc .* (\\")?.*(/Tests/RunCMake/Swift/E.swift|\\\\Tests\\\\RunCMake\\\\Swift\\\\E.swift)(\\")? (\\")?.*(/Tests/RunCMake/Swift/L.swift|\\\\Tests\\\\RunCMake\\\\Swift\\\\L.swift)(\\")?", + "file": ".*(/Tests/RunCMake/Swift/E.swift|\\\\Tests\\\\RunCMake\\\\Swift\\\\E.swift)", + "output": "CMakeFiles/CompileCommandLib.dir/E.swift.o|CMakeFiles\\\\CompileCommandLib.dir\\\\E.swift.obj" +}, +{ + "directory": ".*(/Tests/RunCMake/Swift/CompileCommands-build|\\\\Tests\\\\RunCMake\\\\Swift\\\\CompileCommands-build)", + "command": ".*swiftc .* (\\")?.*(/Tests/RunCMake/Swift/E.swift|\\\\Tests\\\\RunCMake\\\\Swift\\\\E.swift)(\\")? (\\")?.*(/Tests/RunCMake/Swift/L.swift|\\\\Tests\\\\RunCMake\\\\Swift\\\\L.swift)(\\")?", + "file": ".*/Tests/RunCMake/Swift/L.swift", + "output": "CMakeFiles/CompileCommandLib.dir/L.swift.o|CMakeFiles\\\\CompileCommandLib.dir\\\\L.swift.obj" +} +]$]==] +) + +file(READ "${RunCMake_TEST_BINARY_DIR}/compile_commands.json" compile_commands) +if(NOT compile_commands MATCHES "${expected_compile_commands}") + string(REPLACE "\n" "\n " expected_compile_commands_formatted "${expected_compile_commands}") + string(REPLACE "\n" "\n " compile_commands_formatted "${compile_commands}") + string(APPEND RunCMake_TEST_FAILED "Expected compile_commands.json to match:\n ${expected_compile_commands_formatted}\nActual compile_commands.json:\n ${compile_commands_formatted}\n") +endif() diff --git a/Tests/RunCMake/Swift/CompileCommands.cmake b/Tests/RunCMake/Swift/CompileCommands.cmake new file mode 100644 index 0000000..f859693 --- /dev/null +++ b/Tests/RunCMake/Swift/CompileCommands.cmake @@ -0,0 +1,9 @@ +if(POLICY CMP0157) + cmake_policy(SET CMP0157 NEW) +endif() +set(CMAKE_Swift_COMPILATION_MODE "singlefile") + +enable_language(Swift) + +add_library(CompileCommandLib STATIC E.swift L.swift) +set_target_properties(CompileCommandLib PROPERTIES EXPORT_COMPILE_COMMANDS YES) diff --git a/Tests/RunCMake/Swift/RunCMakeTest.cmake b/Tests/RunCMake/Swift/RunCMakeTest.cmake index 184b461..500bf76 100644 --- a/Tests/RunCMake/Swift/RunCMakeTest.cmake +++ b/Tests/RunCMake/Swift/RunCMakeTest.cmake @@ -61,6 +61,11 @@ elseif(RunCMake_GENERATOR STREQUAL Ninja) run_cmake(CMP0157-WARN) endblock() + block() + set(CompileCommands_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/CompileCommands-build) + run_cmake(CompileCommands) + run_cmake_command(CompileCommands-check ${CMAKE_COMMAND} --build ${CompileCommands_TEST_BINARY_DIR}) + endblock() endif() elseif(RunCMake_GENERATOR STREQUAL "Ninja Multi-Config") if(CMake_TEST_Swift) -- cgit v0.12