diff --git a/README.md b/README.md index 8edf06a..53844fe 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ Configuration* sibling = LocalProject({ .projectFile = "../Crafter.Math/project. cfg.dependencies = { math }; ``` -The sibling's `CrafterBuildProject` gets the args this `project.cpp` was loaded with (`ProjectArgs()`), so `--target=`, `--march=`, `--debug` and the project's own flags reach every sibling down the chain. That is all of them: `cfg.projectArgs` holds only what `ApplyStandardArgs` did not interpret, and passing it on would drop the target and the build type. Set `.args` to send something else: `{}` for nothing, or a list built from the project's args, such as one without a flag only this project reads. Build such a list once and give it to every sibling. A project that two paths load with different lists is built twice, and the program ends up with both builds. Releases up to 2.2.0 forward nothing when `.args` is left out. +The sibling's `CrafterBuildProject` gets the args this `project.cpp` was loaded with (`ProjectArgs()`), so `--target=`, `--march=`, `--debug` and the project's own flags reach every sibling down the chain. That is all of them: `cfg.projectArgs` holds only what `ApplyStandardArgs` did not interpret, and passing it on would drop the target and the build type. Set `.args` to send something else: `{}` for nothing, or a list built from the project's args, such as one without a flag only this project reads. Build such a list once and give it to every sibling. A project that two paths load with different lists would end up in the program twice, so the build stops with an error that names both paths. Releases up to 2.2.0 forward nothing when `.args` is left out. The default is read when the spec is made, so make it inside `CrafterBuildProject`, on the thread that calls it. A spec made at namespace scope, on a thread of your own or in a script forwards nothing. diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 6041c8b..35b64d7 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -444,6 +444,42 @@ namespace { } return outcome; } + + // Two configurations among `config` and its transitive dependencies that + // export a module of the same name: the same project loaded along two + // paths with different args, say, or from a checkout on one path and a + // clone on the other. A program can hold one module of each name. Built + // anyway, both PCM dirs went onto the consumer's -fprebuilt-module-path, + // clang resolved the module by name to the first for every import, the + // ones inside the other build's consumers included, and nothing checked + // that it was the build they had been compiled against. The program then + // linked code compiled against two layouts of the same types, and ran + // (issue #41). Empty when the dependencies agree, otherwise the error, + // naming the path to each of the two builds. + std::string DuplicateModuleError(const Configuration& config) { + struct Owner { + const Configuration* cfg; + std::string chain; + }; + const auto describe = [](const Owner& owner) { + return std::format("{} ({}, {}, project args {})", owner.chain, owner.cfg->path.string(), owner.cfg->VariantId(), owner.cfg->projectArgs); + }; + std::unordered_map owners; + std::unordered_set visited; + std::string error; + std::function walk = [&](const Configuration& c, const std::string& chain) { + if (!error.empty() || !visited.insert(&c).second) return; + for (const std::unique_ptr& iface : c.interfaces) { + const auto [it, inserted] = owners.emplace(iface->name, Owner{ &c, chain }); + if (inserted || it->second.cfg == &c) continue; + error = std::format("module {} is built twice for {}:\n {}\n {}\nA program can hold one build of each module: load {} with the same args, and from the same place, along every path.", iface->name, config.name, describe(it->second), describe(Owner{ &c, chain }), c.name); + return; + } + for (const Configuration* dep : c.dependencies) walk(*dep, std::format("{} -> {}", chain, dep->name)); + }; + walk(config, config.name); + return error; + } } BuildResult Crafter::Build(Configuration& config, std::unordered_map>& depResults, std::mutex& depMutex) { @@ -452,6 +488,11 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map a .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); cfg.AddTest("HostToolingSibling").Dependencies({ CrafterBuildLib.get() }) .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); + cfg.AddTest("DuplicateModule").Dependencies({ CrafterBuildLib.get() }) + .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); cfg.AddTest("Script").Dependencies({ CrafterBuildLib.get() }) .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); cfg.AddTest("DependencyNotes").Dependencies({ CrafterBuildLib.get() }) diff --git a/tests/DuplicateModule/fixture/a/interfaces/A.cppm b/tests/DuplicateModule/fixture/a/interfaces/A.cppm new file mode 100644 index 0000000..f2a90a6 --- /dev/null +++ b/tests/DuplicateModule/fixture/a/interfaces/A.cppm @@ -0,0 +1,6 @@ +export module A; +import std; +import M; +export std::int32_t ASize() { + return Size(); +} diff --git a/tests/DuplicateModule/fixture/a/project.cpp b/tests/DuplicateModule/fixture/a/project.cpp new file mode 100644 index 0000000..ecccfd9 --- /dev/null +++ b/tests/DuplicateModule/fixture/a/project.cpp @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// Loads M with the default, so M gets whatever A was given. +extern "C" Configuration CrafterBuildProject(std::span args) { + Configuration* m = LocalProject({ .projectFile = "../m/project.cpp" }); + + Configuration cfg; + cfg.path = "./"; + cfg.name = "A"; + cfg.outputName = "A"; + cfg.type = ConfigurationType::LibraryStatic; + ApplyStandardArgs(cfg, args); + cfg.dependencies = { m }; + std::array ifaces = { "interfaces/A" }; + std::array impls = {}; + cfg.GetInterfacesAndImplementations(ifaces, impls); + return cfg; +} diff --git a/tests/DuplicateModule/fixture/m/interfaces/M.cppm b/tests/DuplicateModule/fixture/m/interfaces/M.cppm new file mode 100644 index 0000000..2906be2 --- /dev/null +++ b/tests/DuplicateModule/fixture/m/interfaces/M.cppm @@ -0,0 +1,12 @@ +export module M; +import std; +// --wide gives S more members, so the two builds of M the test asks for +// differ in what they declare. +#ifdef WIDE +export struct S { std::int32_t a; std::int32_t b; std::int64_t pad[4]; }; +#else +export struct S { std::int32_t a; std::int32_t b; }; +#endif +export std::int32_t Size() { + return static_cast(sizeof(S)); +} diff --git a/tests/DuplicateModule/fixture/m/project.cpp b/tests/DuplicateModule/fixture/m/project.cpp new file mode 100644 index 0000000..5cf21fb --- /dev/null +++ b/tests/DuplicateModule/fixture/m/project.cpp @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// --wide is this project's own flag, and it changes what M declares. +extern "C" Configuration CrafterBuildProject(std::span args) { + Configuration cfg; + cfg.path = "./"; + cfg.name = "M"; + cfg.outputName = "M"; + cfg.type = ConfigurationType::LibraryStatic; + ApplyStandardArgs(cfg, args); + if (std::ranges::contains(args, std::string_view("--wide"))) cfg.compileFlags.push_back("-DWIDE"); + std::array ifaces = { "interfaces/M" }; + std::array impls = {}; + cfg.GetInterfacesAndImplementations(ifaces, impls); + return cfg; +} diff --git a/tests/DuplicateModule/fixture/p/main.cpp b/tests/DuplicateModule/fixture/p/main.cpp new file mode 100644 index 0000000..331b6b6 --- /dev/null +++ b/tests/DuplicateModule/fixture/p/main.cpp @@ -0,0 +1,6 @@ +import std; +import M; +import A; +int main() { + return Size() == ASize() ? 0 : 1; +} diff --git a/tests/DuplicateModule/fixture/p/project.cpp b/tests/DuplicateModule/fixture/p/project.cpp new file mode 100644 index 0000000..3f4a06b --- /dev/null +++ b/tests/DuplicateModule/fixture/p/project.cpp @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// Loads M itself and through A, which passes on all of its args. With +// --split, the list P gives M leaves --wide out, as a filter meant for one +// sibling would, and the two paths load M with two different lists. +extern "C" Configuration CrafterBuildProject(std::span args) { + const bool split = std::ranges::contains(args, std::string_view("--split")); + std::vector mArgs; + for (std::string_view arg : args) { + if (!split || arg != "--wide") mArgs.emplace_back(arg); + } + Configuration* m = LocalProject({ .projectFile = "../m/project.cpp", .args = mArgs }); + Configuration* a = LocalProject({ .projectFile = "../a/project.cpp" }); + + Configuration cfg; + cfg.path = "./"; + cfg.name = "P"; + cfg.outputName = "P"; + cfg.type = ConfigurationType::Executable; + ApplyStandardArgs(cfg, args); + cfg.dependencies = { m, a }; + std::array ifaces = {}; + std::array impls = { "main" }; + cfg.GetInterfacesAndImplementations(ifaces, impls); + return cfg; +} diff --git a/tests/DuplicateModule/main.cpp b/tests/DuplicateModule/main.cpp new file mode 100644 index 0000000..b0bb3f2 --- /dev/null +++ b/tests/DuplicateModule/main.cpp @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: LGPL-3.0-only +// SPDX-FileCopyrightText: Copyright (C) 2026 Catcrafts® + +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// A program can hold one module of each name, so Build() refuses a graph in +// which two configurations export the same module: the same project loaded +// along two paths with two arg lists, say. It used to build both and put +// both on the consumer's module path. Clang took the first for every import +// of the module, the ones inside the other build's consumers included, and +// the program linked code compiled against two layouts of one struct: it +// built cleanly, then read garbage or crashed (issue #41). The fixture is +// P -> M and P -> A -> M. With --split, P gives M its args without --wide, +// which changes what M declares, while A passes them on whole. +namespace { + std::int32_t Failures = 0; + + void Check(bool cond, std::string_view msg) { + if (!cond) { + std::println(std::cerr, "FAIL: {}", msg); + ++Failures; + } + } + + BuildResult BuildPass(Configuration& cfg) { + std::unordered_map> depResults; + std::mutex depMutex; + return Build(cfg, depResults, depMutex); + } +} + +int main() { + const fs::path installedHome = GetCrafterBuildHome(); + if (!fs::exists(installedHome / "Crafter.Build.cppm")) { + std::println(std::cerr, "SKIP: no module sources at {}", installedHome.string()); + return 77; + } + + // Staged because LoadProject and Build drop build/ and bin/ trees next + // to the project files. + const fs::path fixture = fs::current_path() / "tests" / "DuplicateModule" / "fixture"; + const fs::path scratch = fs::temp_directory_path() / "crafter-build-duplicate-module"; + std::error_code ec; + fs::remove_all(scratch, ec); + fs::create_directories(scratch); + fs::copy(fixture, scratch, fs::copy_options::recursive); + + // From P's own directory, as crafter-build runs it: the fixture's + // LocalProject paths are relative to the working directory. + const fs::path cwd = fs::current_path(); + fs::current_path(scratch / "p"); + try { + // Two lists for M: refused before anything is built. + { + std::array args = { "--wide", "--split" }; + Configuration cfg = LoadProject("project.cpp", args); + const BuildResult built = BuildPass(cfg); + Check(!built.result.empty(), "a graph with two builds of one module fails to build"); + Check(built.result.contains("module M is built twice for P"), std::format("the error names the module and the program (got: {})", built.result)); + Check(built.result.contains("P -> M (") && built.result.contains("P -> A -> M ("), std::format("the error names the path to each build (got: {})", built.result)); + Check(!fs::exists(scratch / "m" / "bin") && !fs::exists(scratch / "a" / "bin") && !fs::exists(scratch / "p" / "bin"), "nothing was built"); + } + + // One list along both paths: one M, and the build goes through. + { + std::array args = { "--wide" }; + Configuration cfg = LoadProject("project.cpp", args); + const bool oneM = cfg.dependencies.size() == 2 && cfg.dependencies[1]->dependencies.size() == 1 && cfg.dependencies[0] == cfg.dependencies[1]->dependencies[0]; + Check(oneM, "the same list along both paths loads one M"); + const BuildResult built = BuildPass(cfg); + Check(built.result.empty(), std::format("the graph with one M builds (got: {})", built.result)); + } + } catch (const std::exception& e) { + fs::current_path(cwd); + std::println(std::cerr, "FAIL: threw: {}", e.what()); + return 1; + } + fs::current_path(cwd); + + return Failures > 0 ? 1 : 0; +}