Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2238 - Renew source files after preGenerateCommands #2239

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Fix #2238 - Renew source files after preGenerateCommands #2239

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions source/dub/generators/generator.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,10 @@ class ProjectGenerator
prepareGeneration(pack, m_project, settings, bs);

// Regenerate buildSettings.sourceFiles
if (bs.preGenerateCommands.length)
bs = makeBuildSettings(pack, targets[pack.name].buildSettings);
if (bs.preGenerateCommands.length) {
auto newSettings = pack.getBuildSettings(settings.platform, configs[pack.name]);
bs = makeBuildSettings(pack, newSettings);
}
targets[pack.name].buildSettings = bs;
}
configurePackages(m_project.rootPackage, targets, settings);
Expand Down Expand Up @@ -1051,3 +1053,51 @@ private void storeRecursiveInvokations(ref const(string[string])[] env, string[]
.join(",")
];
}

version(Posix) {
// https://github.com/dlang/dub/issues/2238
unittest {
import dub.internal.vibecompat.data.json : parseJsonString;
import dub.compilers.gdc : GDCCompiler;
import std.algorithm : canFind;
import std.path : absolutePath;
import std.file : mkdirRecurse, rmdirRecurse, write;

mkdirRecurse("dubtest/preGen/source");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the dub unittests write to the filesystem. It would probably be better to use the tests folder test system with shell scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a good enough reason? I think it's a lot better to unit test even if it requires writing to the file system, especially as the only way I can get this to manifest in real life is with two dub packages where one depends on the other.

Copy link
Member

@WebFreak001 WebFreak001 Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my main reason would be that all the existing similar tests use the test folder (shell scripts or D scripts, optionally with their own folders where they can do whatever they want) and I don't think dub has such tests inside the unittests.

Additionally the test runner in the tests folder is more easily usable with isolating tests and debugging issues.

Example test script:
https://github.com/dlang/dub/blob/059f7a2a16cf7a50a5c68dc295c796482116ff87/test/environment-variables.script.d

and test folder:
https://github.com/dlang/dub/tree/059f7a2a16cf7a50a5c68dc295c796482116ff87/test/environment-variables

while this unittest is nice in testing the issue in isolation, (as unittests should do) the test folder in general is quite useful as it runs the whole dub executable with the full generator and stuff, so issues that are related to this, but not exactly this issue, may be found more easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get all that - I'm arguing that the way in this PR is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please merge? I don't think "We've always done it this way" is a valid argument.

write("dubtest/preGen/source/foo.d", "");
scope(exit) rmdirRecurse("dubtest");

auto desc = parseJsonString(`{"name": "test", "targetType": "library", "preGenerateCommands": ["touch $PACKAGE_DIR/source/bar.d"]}`);
auto pack = new Package(desc, NativePath("dubtest/preGen".absolutePath));
auto pman = new PackageManager(pack.path, NativePath("/tmp/foo/"), NativePath("/tmp/foo/"), false);
auto prj = new Project(pman, pack);

final static class TestCompiler : GDCCompiler {
override void invoke(in BuildSettings settings, in BuildPlatform platform, void delegate(int, string) output_callback) {
assert(false);
}
override void invokeLinker(in BuildSettings settings, in BuildPlatform platform, string[] objects, void delegate(int, string) output_callback) {
assert(false);
}
}

GeneratorSettings settings;
settings.compiler = new TestCompiler;
settings.buildType = "debug";

final static class TestGenerator : ProjectGenerator {
this(Project project) {
super(project);
}

override void generateTargets(GeneratorSettings settings, in TargetInfo[string] targets) {
import std.conv : text;
const sourceFiles = targets["test"].buildSettings.sourceFiles;
assert(sourceFiles.canFind("dubtest/preGen/source/bar.d".absolutePath), sourceFiles.text);
}
}

auto gen = new TestGenerator(prj);
gen.generate(settings);
}
}