Enable automatic compiling of imports #7099

marler8997 · 2017-08-25T12:57:52Z

An implementation of an idea I had (forum discussion here: http://forum.dlang.org/thread/tcrdpvqvwxffnewzohuj@forum.dlang.org)

I added a compiler option -ci which stands for "compile imports".

When this option is enabled, the compiler will automatically treat imported modules as if they were specified on the command line so long as it doesn't think they exist in any library. For now it uses the same logic as rdmd to determine whether a module is in a library (see the inALibrary function), which is not full proof, but works well enough for a prototype. With this change you should now be able to omit all the imported module files from the command line, i.e.

Instead of:

dmd prog.d -Isomelib somelib\foo\module1.d somelib\foo\module2.d somelib\foo\module3.d somelib\foo\module4.d somelib\foo\module5.d somelib\foo\module6.d -Ianotherlib anotherlib\bar\module1.d anotherlib\bar\module2.d anotherlib\bar\module3.d anotherlib\bar\module4.d anotherlib\bar\module5.d

Use:

dmd -ci prog.d -Isomelib -Ianotherlib

Should this feature be desirable, I recommend coming up with a more deterministic way of determining what modules exist in object/library files.

Also note that this feature would simplify rdmd and remove the need for it to invoke the compiler twice. This would fix the current performance problem with rdmd and would have even better performance than the previous solution (dlang/tools#194).

dlang-bot · 2017-08-25T12:57:54Z

Thanks for your pull request, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

PetarKirov · 2017-08-25T13:03:31Z

src/ddmd/globals.d

@@ -74,6 +74,7 @@ struct Param
 {
    bool obj;               // write object file
    bool link;              // perform link
+    bool compileimports;    // compile imported modules that have not been passed in


The D modules and the C++ headers need to be kept in sync. You need to update src/ddmd/globals.h too.

done, thanks for catching that.

PetarKirov · 2017-08-25T13:04:26Z

This branch has conflicts that must be resolved

Looks like you need to rebase your branch on top of the latest master. Most probably, this refactoring is the cause for the git conflict. I think you need to move the parsing of the cmd-line to the new parseCommandLine function.

wilzbach

Just a couple of minor nits. Do you need help adding test cases?

wilzbach · 2017-08-26T11:37:54Z

src/ddmd/dmodule.d

@@ -381,6 +381,7 @@ extern (C++) final class Module : Package
    // i.e. a module that will be taken all the
    // way to an object file
    Module importedFrom;
+    bool compiledImport;


A documentation comment wouldn't hurt

wilzbach · 2017-08-26T11:41:02Z

src/ddmd/mars.d

+    {
+        auto importedModulesToCompile = Modules();
+        fprintf(global.stdmsg, "compileimports: start\n");
+        for (size_t i = 0; i < Module.amodules.dim; i++)


Don't be afraid of using D features - the current codebase just doesn't in many places because it was automatically transpiled from C++

foreach(i; 0 .. Module.amodules.dim)

I did a search in all of ddmd and found like 1 or 2 instances of foreach which really surprised me. Good to know it's ok to use the D feature in this case.

wilzbach · 2017-08-26T11:43:32Z

src/ddmd/mars.d

+                continue;
+            }
+            fprintf(global.stdmsg, "compileimports: compiling \"%s\" (file \"%s\" importedFrom \"%s\")\n",
+                m.toChars(), m.srcfile.toChars, m.importedFrom.toChars());


For Phobos the coding style is to avoid unnecessary parentheses, for DMD there isn't a style guide yet, but it's probably a good idea to be consistent within the same line

JohanEngelen · 2017-08-26T13:15:23Z

I added a compiler option -ci which stands for "compile imports".

So please name it -compile-imports.

marler8997 · 2017-08-26T13:34:41Z

@wilzbach I was planning on adding test cases when I got some response from a maintainer that this was a desirable feature. If you'd like to help guide/contribute to any cases then advice and PR's are very welcome (Note I'm not that familiar with the test infrastructure for dmd).

ibuclaw · 2017-09-28T21:21:00Z

Isn't this what rdmd is for?

marler8997 · 2017-09-28T21:32:04Z

@ibuclaw It's part of what rdmd does (another big thing rdmd does is automatically cache output files, which the compiler does not do). This PR moves that part of rdmd into the compler itself. One big advantage of this is it solves the performance problems we have with rdmd since it currently has to invoke the compiler twice, once to get the imports and once to compile with them. This feature allows rdmd to tell the compiler to just include the missing imports, no need to invoke the compiler again. It also makes alot of other use cases much simpler when building programs without rdmd.

JinShil · 2017-12-02T06:36:10Z

@andralex and @WalterBright
I think we can carry something like this across the finish line, but we need to hear from you if it is something you want to have and support. Are DIPs required for simple things like this (additional compiler switches)? This isn't really a language change.

@marler8997
A thorough rationale statement would help @andralex and @WalterBright make a decision.

ibuclaw · 2017-12-02T08:59:23Z

I think that the implementation should be part of the driver, not the frontend. All changes should be moved to mars.d

andralex · 2017-12-02T18:03:01Z

There must be a clear case for impact, i.e. to what extent this feature is superior and desirable to rdmd. Alternatively, it should be clarified how this tool could simplify rdmd to eliminate redundant logic. As things are, this feature competes with a subset of rdmd while still making it necessary.

marler8997 · 2017-12-02T18:13:00Z

@andralex

There must be a clear case for impact, i.e. to what extent this feature is superior and desirable to rdmd.

The impact to rdmd is that it doesn't have to invoke the compiler twice. However this feature isn't just about rdmd, it's also about making compilation simpler by invoking the DRY principle. Instead of maintaining a list of modules to compile as well as a list of imports, you use the imports themselves to determine what needs to be compiled. A side effect of this is that rdmd can use this feature instead of invoking the compiler twice but I wouldn't say that is the primary benefit.

andralex · 2017-12-02T18:42:24Z

The impact to rdmd is that it doesn't have to invoke the compiler twice.

How long does the first invocation take?

However this feature isn't just about rdmd, it's also about making compilation simpler by invoking the DRY principle.

That's also the charter of rdmd.

wilzbach · 2017-12-02T18:59:17Z

How long does the first invocation take?

For a single file, it's about 40% of the runtime.
We already had this discussion last summer (see e.g. dlang/tools#191 or dlang/tools#194). In fact, at that point we merged a PR that avoids the double invocation for the trivial case of having no dependencies (this PR was reverted later due to a regression and no one having time to look into it). Anyhow to quote from the last discussion:

The correct solution would be to move rdmd into dmd. IIRC Andrej attempted this a while ago, but hit a snag.

So while this PR isn't ready for prime time yet, I do think that the idea itself is a worthwhile goal and could become quite powerful due to the performance savings.

CCing the people who have been involved in the last discussion: @CyberShadow @AndrejMitrovic @aG0aep6G @mihails-strasuns

mihails-strasuns · 2017-12-03T01:52:14Z

AFAIR main purpose of rdmd was to support efficient shebang script usage (with caching resulting binary instead of recompilation each time it used). Compilation of dependencies came as a necessary requirement to make it work, not as primary functionality. I agree that it makes more sense to put it in compiler while focusing rdmd on its original purpose.

CyberShadow · 2017-12-03T17:42:57Z

Thanks for working on this, @marler8997! I agree that this is a logical change that should have been made long ago, and it will improve the quality of life for all D developers not using Dub to build their code.

One point in previous discussions is that there should be some way to specify exclusions, for packages that are going to be linked in as libraries and don't need to be complied again. This applies to Phobos/Druntime (std.*, core.* and etc.*), but ought to be customizable.

CyberShadow · 2017-12-03T18:43:42Z

So please name it -compile-imports.

Disagree. The addition of this option has the potential to obsolete rdmd, and it is usable directly as well as by other build tools, so a short switch would be a boon for this functionality.

marler8997 · 2017-12-03T18:56:45Z

@CyberShadow

Thanks for chiming in and showing support. I had the same thoughts about exclusions. Another thought I had was when you enable "compile imports", the compiler could read the object/library files and figure out which modules they include based on the existence of a particular symbol. I believe there is a special symbol that each module generates, something like "ModuleInfo". This would add overhead to the compiler to read those files, not sure how much of an impact it would have on performance, but this is the most robus solution I've come up with.

CyberShadow · 2017-12-03T18:58:39Z

This would add overhead to the compiler to read those files, not sure how much of an impact it would have on performance, but this is the most robus solution I've come up with.

Wouldn't this mean that the compiler would need to read and parse the object/library files, rather than just passing them to the linker? Sounds like a lot of work.

ibuclaw · 2018-01-14T19:18:46Z

On the high level aspect, I'm not particularly a big fan. I really don't like the idea of conflating compiler and build system.

On technical, fine. Its isolated enough, though I have alluded to Compiler.loadModule a couple of times. However there are things that need to be sorted out before that callback is usable, so there's no reason to block on that (just an FYI as a potential future refactor).

Dismiss

wilzbach · 2018-01-15T06:20:30Z

(closed & re-opened, s.t. the newly added SemaphoreCI can find this PR)

MartinNowak · 2018-01-20T19:39:11Z

The approach to push more modules to semantic3 might easily introduce subtle bugs, e.g. by breaking assumptions about a fixed set of root modules or by missing semantic steps.
Let's try to test this feature enough so that we can find those before releasing it.

marler8997 · 2018-01-20T19:47:30Z

Most of the work done for this feature was in the dmd test suite. The test suite was modified to allow a test to be run twice, with and without -i. That being said, I have set the "isRoot" property before any other code checks it. It's done when constructing the module, just after it is parsed. It was also important to do this before any semantic analysis was done as I discovered that some of the semantic analysis runs differently depending on if the module is a root module.

Furthermore, the list of root modules is a local variable in mars.d. Every usage of it is in that file so checking whether or not someone could be making assumptions about it is limited to verifying mars.d which I carefully attempted to do.

Also, performing extra semantic analysis on more modules than just the initial ones passed on the command line is already being done when the -deps flag is enabled. It doesn't add new root modules but it does cover some of that test surface.

WalterBright · 2018-09-05T10:01:52Z

src/dmd/mars.d

+// array of module patterns used to include/exclude imported modules
+private __gshared Array!(const(char)*) includeModulePatterns;
+private __gshared Modules compiledImports;
+private extern(C++) bool marsOnImport(Module m)


Why are we approving things with no Ddoc function headers?

WalterBright · 2018-09-05T10:02:34Z

src/dmd/mars.d

@@ -2125,3 +2168,278 @@ private bool parseCommandLine(const ref Strings arguments, const size_t argc, re
    return errors;
 }

+
+private __gshared bool includeImports = false;


This random assortment of undocumented globals is more technical debt.

This boolean and array represent all the data we need to know "compile imports" is enabled and all the patterns that were given. Are you wanting them to be put in a struct as static variables? What's the "debt" you're seeing that should be alleviated?

WalterBright · 2018-09-05T10:03:02Z

src/dmd/mars.d

@@ -2125,3 +2168,278 @@ private bool parseCommandLine(const ref Strings arguments, const size_t argc, re
    return errors;
 }

+
+private __gshared bool includeImports = false;
+// array of module patterns used to include/exclude imported modules


I have no idea what a "module pattern" is.

A module pattern is a sequence of identifiers that are matched against the first part of a any module name. I tried to clearly define it in the associated documentation https://dlang.org/dmd-windows.html#switch-i[

WalterBright · 2018-09-05T10:03:44Z

src/dmd/mars.d

+        if (index < packages.dim)
+            return (*packages)[index];
+        if (index == packages.dim)
+            return name;


what's going on here? There's no mention of this in the comments.

WalterBright · 2018-09-05T10:04:46Z

src/dmd/mars.d

+
+/*
+ * Determines if the given module should be included in the compilation.
+ * Returns:


How can a ModuleComponentRange be considered a "given module" ?

No Params: section.

WalterBright · 2018-09-05T10:05:34Z

src/dmd/mars.d

+}
+
+// Matching module names is done with an array of matcher nodes.
+// The nodes are sorted by "component depth" from largest to smallest


What is a "component depth"?

The "depth" is the number of components in a module name and/or pattern (i.e. foo.bar.baz has 3 components, so a depth of 3). It's been so long I'm not sure why the term "depth" was chosen.

WalterBright · 2018-09-05T10:05:59Z

src/dmd/mars.d

+        struct
+        {
+            ushort depth;
+            bool isExclude;


what does isExclude for?

isExclude indicates whether the pattern is an "exclusive" (i.e. -i=-foo.bar) pattern, vs an "inclusive" one (i.e. -i=foo.bar). The documentation will explain better, but in essence, patterns are prioritized by their component length and are either "inclusive" or "exclusive". Say you want to include all modules from package foo except for all modules in the foo.bar sub-path.

dmd -i=foo -i=-foo.bar

Then foo would be an inclusive pattern and -i=-foo.bar would be an exclusive pattern. Since the algorithm works by sorting these "module patterns" by their component length, the "foo.bar" pattern will be checked first, giving the desired effect.

If you want the opposite, which is to include all modules in "foo.bar", you can simply give

dmd -i=foo.bar

Looking at the code, I guess you were probably more wondering what the MatcherNode is used for. The full pattern-matching algorithm uses an array of matcher nodes. This array of matcher nodes is a representation of all the patterns given to the compiler. Each "pattern" has a MatcherNode that indicates how many components it has and whether it is an exclude pattern (via isExclude) followed by a number of "Identifier" MatcherNode's which provide the identifiers for the pattern.

For example, the following "-i=-foo.bar -i=foo" would become:

Index Node

0 MatcherNode(depth=2, isExclude=true)

1 MatcherNode(Id.foo)

2 MatcherNode(Id.bar)

3 MatcherNode(depth=1, isExclude=false)

1 MatcherNode(Id.foo)

However I think you're main point is that this needs to be documented?

WalterBright · 2018-09-05T10:07:03Z

src/dmd/mars.d

+
+/*
+ * $(D includeByDefault) determines whether to include/exclude modules when they don't
+ * match any pattern. This setting changes depending on if the user provided any "inclusive" module


what is meant by "pattern"?

https://dlang.org/dmd-windows.html#switch-i[

This behavior can be overriden by providing patterns via -i=. A pattern of the form -i= is an "inclusive pattern", whereas a pattern of the form -i=- is an "exclusive pattern". Inclusive patterns will include all module's whose names match the pattern, whereas exclusive patterns will exclude them. For example. all modules in the package foo.bar can be included using -i=foo.bar or excluded using -i=-foo.bar. Note that each component of the fully qualified name must match the pattern completely, so the pattern foo.bar would not match a module named foo.barx.

marler8997 · 2018-09-05T13:41:16Z

@WalterBright
It sounds like most of your comments are about lack of comments. If you like I can go through this code and create a PR to fill out any missing comments?

marler8997 · 2018-09-05T14:26:20Z

src/dmd/mars.d

+        if (index == packages.dim)
+            return name;
+        else
+            return Identifier.idPool("package");


This should probably just return Id.package, I'd be surprised if package wasn't already a pre-allocated identifier.

nordlow · 2020-06-16T15:58:12Z

Does this feature support caching of binaries indexed through hashing of inputs and all its (transitive) dependencies like rdmd does? If not are there any plans for doing this?

wilzbach · 2020-06-16T21:50:11Z

No it doesn't do caching and there are no plans to add a build system to the compiler.
If you want a better rdmd, you should have a look at https://github.com/dragon-lang/rund

marler8997 force-pushed the compileimports branch from cc4c374 to 572167b Compare August 25, 2017 12:58

PetarKirov reviewed Aug 25, 2017

View reviewed changes

marler8997 force-pushed the compileimports branch 2 times, most recently from dd91cc1 to ce11470 Compare August 25, 2017 13:36

wilzbach reviewed Aug 26, 2017

View reviewed changes

marler8997 force-pushed the compileimports branch 5 times, most recently from 55dec74 to 5730200 Compare August 26, 2017 13:50

JinShil assigned WalterBright Dec 2, 2017

JinShil added the Review:Needs Approval label Dec 2, 2017

marler8997 force-pushed the compileimports branch from 5730200 to c04e1c5 Compare December 3, 2017 02:22

marler8997 force-pushed the compileimports branch from c630799 to 4eec8ca Compare January 14, 2018 20:41

Added the "-i" (include imports) command line option.

4eec8ca

andralex added the Merge:auto-merge label Jan 14, 2018

wilzbach closed this Jan 15, 2018

wilzbach reopened this Jan 15, 2018

dlang-bot merged commit 7964ab3 into dlang:master Jan 15, 2018

This was referenced Jan 18, 2018

rdmd runs dmd twice for single-files with no dependencies wilzbach/tools-test#144

Closed

rdmd does not discover all dependencies wilzbach/tools-test#180

Closed

marler8997 mentioned this pull request Jan 19, 2018

Document the new -i dmd compiler option dlang/dlang.org#2109

Closed

marler8997 deleted the compileimports branch January 27, 2018 09:54

marler8997 mentioned this pull request Apr 5, 2018

Isolate tests to one file using HAR #7905

Closed

WalterBright reviewed Sep 5, 2018

View reviewed changes

marler8997 commented Sep 5, 2018

View reviewed changes

dlang-bugzilla-migration mentioned this pull request Jun 5, 2020

rdmd runs dmd twice for single-files with no dependencies dlang/tools#413

Open

dlang-bugzilla-migration mentioned this pull request Jan 9, 2021

rdmd does not discover all dependencies dlang/tools#423

Open

Index	Node
0	MatcherNode(depth=2, isExclude=true)
1	MatcherNode(Id.foo)
2	MatcherNode(Id.bar)
3	MatcherNode(depth=1, isExclude=false)
1	MatcherNode(Id.foo)

Enable automatic compiling of imports #7099

Enable automatic compiling of imports #7099

Conversation

marler8997 commented Aug 25, 2017 • edited Loading

dlang-bot commented Aug 25, 2017

Bugzilla references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov commented Aug 25, 2017 • edited Loading

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Aug 26, 2017

marler8997 commented Aug 26, 2017

ibuclaw commented Sep 28, 2017

marler8997 commented Sep 28, 2017

JinShil commented Dec 2, 2017 • edited Loading

ibuclaw commented Dec 2, 2017

andralex commented Dec 2, 2017

marler8997 commented Dec 2, 2017

andralex commented Dec 2, 2017

wilzbach commented Dec 2, 2017 • edited Loading

mihails-strasuns commented Dec 3, 2017

CyberShadow commented Dec 3, 2017

CyberShadow commented Dec 3, 2017

marler8997 commented Dec 3, 2017

CyberShadow commented Dec 3, 2017

ibuclaw commented Jan 14, 2018

wilzbach commented Jan 15, 2018

MartinNowak commented Jan 20, 2018

marler8997 commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

marler8997 Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 commented Sep 5, 2018

Choose a reason for hiding this comment

nordlow commented Jun 16, 2020 • edited Loading

wilzbach commented Jun 16, 2020

marler8997 commented Aug 25, 2017 •

edited

Loading

PetarKirov commented Aug 25, 2017 •

edited

Loading

JinShil commented Dec 2, 2017 •

edited

Loading

wilzbach commented Dec 2, 2017 •

edited

Loading

marler8997 Sep 5, 2018 •

edited

Loading

marler8997 Sep 5, 2018 •

edited

Loading

marler8997 Sep 5, 2018 •

edited

Loading

marler8997 Sep 5, 2018 •

edited

Loading

marler8997 Sep 5, 2018 •

edited

Loading

nordlow commented Jun 16, 2020 •

edited

Loading