From 32d02e404d46047365bf366a45583bbf442badc7 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Tue, 20 Feb 2024 20:21:41 +0100 Subject: [PATCH 1/7] dub: Cache PackageManager across dub{Package,Dependant} targets --- payload/reggae/dub/interop/dublib.d | 26 +++++++++++++++++++++++++- tests/it/runtime/dependencies.d | 12 ++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/payload/reggae/dub/interop/dublib.d b/payload/reggae/dub/interop/dublib.d index 45ead868..f145a38c 100644 --- a/payload/reggae/dub/interop/dublib.d +++ b/payload/reggae/dub/interop/dublib.d @@ -267,9 +267,33 @@ package struct Dub { // module that don't do that on purpose for speed reasons. auto fullDub(in string projectPath) @trusted { import dub.dub: DubClass = Dub; + import dub.packagemanager: PackageManager; import dub.internal.vibecompat.inet.path: NativePath; - auto dub = new DubClass(projectPath); + // Cache the PackageManager. + // A reggaefile.d with lots of dub{Package,Dependant} targets benefits from + // this, also depending on the size of the dub packages cache. + static class DubWithCachedPackageManager : DubClass { + this(string rootPath) { + super(rootPath); + } + + override PackageManager makePackageManager() const { + static PackageManager cachedPM = null; + if (!cachedPM) { + // The PackageManager wants a path to a local directory, for an + // implicit `/.dub/packages` repo. The base + // implementation uses the dub root project directory; use the + // reggae project directory as our local root. + import reggae.config: options; + auto localRoot = NativePath(options.projectPath); + cachedPM = new PackageManager(localRoot, m_dirs.userPackages, m_dirs.systemSettings, false); + } + return cachedPM; + } + } + + auto dub = new DubWithCachedPackageManager(projectPath); dub.packageManager.getOrLoadPackage(NativePath(projectPath)); dub.loadPackage(); dub.project.validate(); diff --git a/tests/it/runtime/dependencies.d b/tests/it/runtime/dependencies.d index 05ad73d2..6a3da1db 100644 --- a/tests/it/runtime/dependencies.d +++ b/tests/it/runtime/dependencies.d @@ -224,10 +224,14 @@ unittest { ] ); - // but do write configuration if dub cfg has changed - auto lines = runIt("-d myvar=foo"); - srcLine.should.not.be in lines; - cfgLine.should.be in lines; + // we cache dub's PackageManager per-thread; use a new thread to make sure the changed .sdl is reloaded + import core.thread: Thread; + new Thread(() { + // but do write configuration if dub cfg has changed + auto lines = runIt("-d myvar=foo"); + srcLine.should.not.be in lines; + cfgLine.should.be in lines; + }).start().join(); } } } From 02675fae4b140f205fd73912794a45d7137eeb75 Mon Sep 17 00:00:00 2001 From: Atila Neves Date: Wed, 28 Feb 2024 10:21:38 +0100 Subject: [PATCH 2/7] Align time output --- payload/reggae/io.d | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/payload/reggae/io.d b/payload/reggae/io.d index ed43fc9a..5a27364e 100644 --- a/payload/reggae/io.d +++ b/payload/reggae/io.d @@ -20,7 +20,8 @@ void log(O, T...)(auto ref O output, auto ref T args) { private string secondsSinceStartString() @safe { import std.string: rightJustify; import std.conv: to; - return ("+" ~ (sinceStart / 1000.0).to!string).rightJustify(8, ' '); + import std.format: format; + return ("+" ~ (sinceStart / 1000.0).format!"%03.3f"()).rightJustify(8, ' '); } From 67e7b506ad49ece2a8f943a77035f86d8e8274ee Mon Sep 17 00:00:00 2001 From: Atila Neves Date: Wed, 28 Feb 2024 11:08:01 +0100 Subject: [PATCH 3/7] Only create GeneratorSettings once since it takes so long --- payload/reggae/dub/interop/dublib.d | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/payload/reggae/dub/interop/dublib.d b/payload/reggae/dub/interop/dublib.d index 45ead868..7d8c5a4a 100644 --- a/payload/reggae/dub/interop/dublib.d +++ b/payload/reggae/dub/interop/dublib.d @@ -35,10 +35,12 @@ package struct Dub { import reggae.dub.info: DubInfo; import reggae.options: Options; import dub.dub: DubClass = Dub; + import dub.generators.generator: GeneratorSettings; private DubClass _dub; private const string[] _extraDFlags; private const(Options) _options; + private GeneratorSettings _generatorSettings; this(in Options options) @trusted { import reggae.path: buildPath; @@ -48,6 +50,7 @@ package struct Dub { _options = options; _dub = fullDub(options.projectPath); _extraDFlags = options.dflags.dup; + _generatorSettings = getGeneratorSettings(options); } const(Options) options() @safe @nogc pure nothrow const return scope { @@ -150,12 +153,11 @@ package struct Dub { import std.conv: text; auto singleConfig = _options.dubConfig; - auto settings = getGeneratorSettings(_options); const allConfigs = singleConfig == ""; // add the special `dub test` configuration (which doesn't require an existing `unittest` config) const lookingForUnitTestsConfig = allConfigs || singleConfig == "unittest"; const testConfig = lookingForUnitTestsConfig - ? _dub.project.addTestRunnerConfiguration(settings) + ? _dub.project.addTestRunnerConfiguration(_generatorSettings) : null; // skip when requesting a single non-unittest config // error out if the test config is explicitly requested but not available @@ -164,7 +166,7 @@ package struct Dub { } const haveSpecialTestConfig = testConfig.length && testConfig != "unittest"; - const defaultConfig = _dub.project.getDefaultConfiguration(settings.platform); + const defaultConfig = _dub.project.getDefaultConfiguration(_generatorSettings.platform); // A violation of the Law of Demeter caused by a dub bug. // Otherwise _dub.project.configurations would do, but it fails for one @@ -174,7 +176,7 @@ package struct Dub { .rootPackage .recipe .configurations - .filter!(c => c.matchesPlatform(settings.platform)) + .filter!(c => c.matchesPlatform(_generatorSettings.platform)) .map!(c => c.name) ; @@ -252,7 +254,7 @@ package struct Dub { private DubInfo configToDubInfo(in string config = "") @trusted /*dub*/ { auto generator = new InfoGenerator(_dub.project, _extraDFlags); - auto settings = getGeneratorSettings(_options); + auto settings = _generatorSettings; settings.config = config; generator.generate(settings); return DubInfo(generator.dubPackages, _options.dup); From acfa4da5845b40baf4049e488f953ae26b5ddce4 Mon Sep 17 00:00:00 2001 From: Atila Neves Date: Wed, 28 Feb 2024 11:58:01 +0100 Subject: [PATCH 4/7] Delete configurations module --- payload/reggae/dub/interop/configurations.d | 11 ----------- payload/reggae/dub/interop/dublib.d | 11 ++++++----- 2 files changed, 6 insertions(+), 16 deletions(-) delete mode 100644 payload/reggae/dub/interop/configurations.d diff --git a/payload/reggae/dub/interop/configurations.d b/payload/reggae/dub/interop/configurations.d deleted file mode 100644 index 645f07f3..00000000 --- a/payload/reggae/dub/interop/configurations.d +++ /dev/null @@ -1,11 +0,0 @@ -module reggae.dub.interop.configurations; - - -@safe: - - -struct DubConfigurations { - string[] configurations; - string default_; - string test; // special `dub test` config -} diff --git a/payload/reggae/dub/interop/dublib.d b/payload/reggae/dub/interop/dublib.d index 7d8c5a4a..68ce0c35 100644 --- a/payload/reggae/dub/interop/dublib.d +++ b/payload/reggae/dub/interop/dublib.d @@ -29,9 +29,13 @@ static this() nothrow { } } +private struct DubConfigurations { + string[] configurations; + string default_; + string test; // special `dub test` config +} package struct Dub { - import reggae.dub.interop.configurations: DubConfigurations; import reggae.dub.info: DubInfo; import reggae.options: Options; import dub.dub: DubClass = Dub; @@ -130,10 +134,7 @@ package struct Dub { return ret; } - imported!"reggae.dub.interop.configurations".DubConfigurations - dubConfigurations(O)(ref O output) - { - import reggae.dub.interop.configurations: DubConfigurations; + DubConfigurations dubConfigurations(O)(ref O output) { import reggae.io: log; output.log("Getting dub configurations"); From ee9019260b7c3e2d30df610481a85deea5ab56ac Mon Sep 17 00:00:00 2001 From: Atila Neves Date: Wed, 28 Feb 2024 12:02:51 +0100 Subject: [PATCH 5/7] Move haveTestConfig to DubConfigurations --- payload/reggae/dub/interop/dublib.d | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/payload/reggae/dub/interop/dublib.d b/payload/reggae/dub/interop/dublib.d index 68ce0c35..222958d4 100644 --- a/payload/reggae/dub/interop/dublib.d +++ b/payload/reggae/dub/interop/dublib.d @@ -33,6 +33,10 @@ private struct DubConfigurations { string[] configurations; string default_; string test; // special `dub test` config + + bool haveTestConfig() @safe @nogc pure nothrow scope const { + return test != ""; + } } package struct Dub { @@ -83,12 +87,11 @@ package struct Dub { DubInfo[string] ret; const configs = dubConfigurations(output); - const haveTestConfig = configs.test != ""; bool atLeastOneConfigOk; Exception dubInfoFailure; foreach(config; configs.configurations) { - const isTestConfig = haveTestConfig && config == configs.test; + const isTestConfig = configs.haveTestConfig && config == configs.test; try { ret[config] = configToDubInfo(output, config, isTestConfig); atLeastOneConfigOk = true; @@ -109,7 +112,7 @@ package struct Dub { // (additionally) expose the special `dub test` config as // `unittest` config in the DSL (`configToDubInfo`) (for // `dubTest!()`, `dubBuild!(Configuration("unittest"))` etc.) - if(haveTestConfig && configs.test != "unittest" && configs.test in ret) + if(configs.haveTestConfig && configs.test != "unittest" && configs.test in ret) ret["unittest"] = ret[configs.test]; return ret; From bad5653152889c66425a9eedd0e630bcd7b2a9ef Mon Sep 17 00:00:00 2001 From: Atila Neves Date: Wed, 28 Feb 2024 12:05:38 +0100 Subject: [PATCH 6/7] Move isTestConfig to DubConfigurations --- payload/reggae/dub/interop/dublib.d | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/payload/reggae/dub/interop/dublib.d b/payload/reggae/dub/interop/dublib.d index 222958d4..50b7f15e 100644 --- a/payload/reggae/dub/interop/dublib.d +++ b/payload/reggae/dub/interop/dublib.d @@ -37,6 +37,10 @@ private struct DubConfigurations { bool haveTestConfig() @safe @nogc pure nothrow scope const { return test != ""; } + + bool isTestConfig(in string config) @safe @nogc pure nothrow scope const { + return haveTestConfig && config == test; + } } package struct Dub { @@ -91,9 +95,8 @@ package struct Dub { Exception dubInfoFailure; foreach(config; configs.configurations) { - const isTestConfig = configs.haveTestConfig && config == configs.test; try { - ret[config] = configToDubInfo(output, config, isTestConfig); + ret[config] = configToDubInfo(output, config, configs.isTestConfig(config)); atLeastOneConfigOk = true; } catch(Exception ex) { output.log("ERROR: Could not get info for configuration ", config, ": ", ex.msg); From 0a583c324e865d10b7ebf7e7a2294be1847621f7 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 28 Feb 2024 18:20:27 +0100 Subject: [PATCH 7/7] Handle removed/renamed build.ninja/Makefile input *files* too, not just source dirs --- payload/reggae/backend/make.d | 12 ++++++---- payload/reggae/backend/ninja.d | 43 +++++++++++++++++----------------- tests/it/runtime/issues.d | 39 ++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 26 deletions(-) diff --git a/payload/reggae/backend/make.d b/payload/reggae/backend/make.d index c8f0a371..62919473 100644 --- a/payload/reggae/backend/make.d +++ b/payload/reggae/backend/make.d @@ -75,9 +75,9 @@ struct Makefile { //includes rerunning reggae string output() @safe { - import std.array: join, replace; + import std.array: join; import std.range: chain; - import std.algorithm: sort, uniq, map; + import std.algorithm: sort, uniq; auto ret = simpleOutput; @@ -87,11 +87,15 @@ struct Makefile { // add a dependency on the Makefile to reggae itself and the build description, // but only if not exporting a build auto srcDirs = _srcDirs.sort.uniq; + const flattenedInputs = chain(options.reggaeFileDependencies, srcDirs).join(" "); const rerunLine = "\t" ~ options.rerunArgs.join(" ") ~ "\n"; - ret ~= fileName() ~ ": " ~ chain(options.reggaeFileDependencies, srcDirs.save).join(" ") ~ "\n"; + ret ~= fileName() ~ ": " ~ flattenedInputs ~ "\n"; ret ~= rerunLine; - ret ~= "\n" ~ srcDirs.save.map!(d => d ~ ":" ~ "\n" ~ rerunLine).join("\n"); + // Add a dummy target for all Makefile inputs, with an empty recipe, just so + // that re-generating the Makefile isn't blocked by removed/renamed inputs. + // See ninja.d for more details. + ret ~= "\n" ~ flattenedInputs ~ ": ;\n"; } return replaceEnvVars(ret); diff --git a/payload/reggae/backend/ninja.d b/payload/reggae/backend/ninja.d index 3d2014ae..7bff0f99 100644 --- a/payload/reggae/backend/ninja.d +++ b/payload/reggae/backend/ninja.d @@ -42,29 +42,28 @@ struct Ninja { import std.algorithm: sort, uniq, map; import std.range: chain, only; - auto srcDirs = _srcDirs.sort.uniq; - const files = flattenEntriesInBuildLine( - chain(_options.reggaeFileDependencies, srcDirs).array - ); - auto paramLines = _options.oldNinja ? [] : ["pool = console"]; - const(NinjaEntry)[] rerunEntries() { // if exporting the build system, don't include rerunning reggae if(_options.export_) return []; - auto rerun = NinjaEntry("build build.ninja: _rerun | " ~ files, paramLines); - // the reason this is needed is because source directories - // can be deleted or renamed. If they are, ninja will - // complain about the missing directories since they are a - // dependency of the build.ninja file for rerunning - // reggae. So we consider them phony targets with no - // dependencies to make that not happen and ninja will - // rerun reggae and change the list of source directories - // accordingly. - auto phonySrcDirs = srcDirs - .map!(a => NinjaEntry("build " ~ a.escapeBuildLine ~ ": phony")) - ; - return chain(rerun.only, phonySrcDirs).array; + + auto srcDirs = _srcDirs.sort.uniq; + const flattenedInputs = flattenEntriesInBuildLine( + chain(_options.reggaeFileDependencies, srcDirs).array + ); + auto paramLines = _options.oldNinja ? [] : ["pool = console"]; + + auto rerun = NinjaEntry("build build.ninja: _rerun | " ~ flattenedInputs, paramLines); + // the reason this is needed is because inputs (files and + // source directories) can be deleted or renamed. If they are, + // ninja will complain about the missing files/directories + // since they are a dependency of the build.ninja file for + // rerunning reggae. So use a dummy phony target 'outputting' + // all build.ninja inputs, so that ninja will happily rerun + // reggae, which will generate a new build.ninja with an + // updated list of build.ninja inputs. + auto phonyInputs = NinjaEntry("build " ~ flattenedInputs ~ ": phony"); + return [rerun, phonyInputs]; } const defaultOutputs = _build.defaultTargetsOutputs(_projectPath); @@ -377,7 +376,7 @@ private: import std.algorithm: map; import std.array: join, replace; return entries - .map!escapeBuildLine + .map!escapePathInBuildLine .join(" "); } @@ -405,9 +404,9 @@ private: } } -private string escapeBuildLine(in string line) @safe pure { +private string escapePathInBuildLine(in string path) @safe pure { import std.array: replace; - return line.replace(":", "$:").replace(" ", "$ "); + return path.replace(":", "$:").replace(" ", "$ "); } struct NinjaEntry { diff --git a/tests/it/runtime/issues.d b/tests/it/runtime/issues.d index 219324a3..1426017c 100644 --- a/tests/it/runtime/issues.d +++ b/tests/it/runtime/issues.d @@ -520,5 +520,44 @@ version(DigitalMars) { mixin(backend).shouldFailToExecute.shouldNotContain(msg); } } + + @("rerun.deleted.file." ~ backend) + @Tags(backend) + unittest { + import std.file: remove; + + with(immutable ReggaeSandbox()) { + + writeFile( + "imported.d", + q{ + import reggae; + enum myTarget = Target.phony("dummy", ""); + } + ); + writeFile( + "reggaefile.d", + q{ + import imported, reggae; + mixin build!myTarget; + } + ); + + runReggae("-b", backend, "--verbose"); + mixin(backend).shouldExecuteOk; + + // delete imported.d and make sure reggae gets rerun + remove(inSandboxPath("imported.d")); + + static if(backend == "ninja") + enum msg = "missing and no known rule"; + else static if(backend == "make") + enum msg = "No rule to make target"; + else + static assert(false, "unknown backend"); + + mixin(backend).shouldFailToExecute.shouldNotContain(msg); + } + } } }