Fix issue 16746 - Please output Makefile-style depfiles for ninja and make by eigenraven · Pull Request #6961 · dlang/dmd

eigenraven · 2017-07-03T12:24:13Z

This adds -M, -Mffile, -Mddirectory, -Mttarget commandline options akin to C(++) compilers, and implements generation of make depfiles which can be used by buildsystems such as Ninja, Make or Meson.

Link to issue: https://issues.dlang.org/show_bug.cgi?id=16746

dlang-bot · 2017-07-03T12:24:15Z

Thanks for your pull request and interest in making D better, @KubaSz! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	16746	enhancement	Please output Makefile-style depfiles for ninja and make

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#6961"

wilzbach · 2017-07-05T00:23:38Z

Seems like gdc doesn't like this change:

.. compilable/makedeps.d          -M -MF=test_results/compilable/makedeps.dep ()
*** glibc detected *** ../src/dmd: double free or corruption (!prev): 0x0000000001fc7280 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x7da66)[0x2b5c8318ca66]
../src/dmd[0x63a5f1]
../src/dmd[0x65c9dd]
../src/dmd[0x660166]
../src/dmd[0x66046e]
../src/dmd[0x64060e]
../src/dmd[0x6376a5]
../src/dmd[0x637719]
../src/dmd[0x63730f]
../src/dmd[0x6374f2]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x2b5c831307ed]
../src/dmd[0x403c09]
======= Memory map: ========
00400000-00782000 r-xp 00000000 fc:06 17744159                           /home/travis/build/dlang/dmd/src/dmd
00782000-007d2000 rw-p 00382000 fc:06 17744159                           /home/travis/build/dlang/dmd/src/dmd
007d2000-007df000 rw-p 00000000 00:00 0 
01f41000-01fd6000 rw-p 00000000 00:00 0                                  [heap]
2b5c829d4000-2b5c829f6000 r-xp 00000000 fc:06 109174333                  /lib/x86_64-linux-gnu/ld-2.15.so

wilzbach · 2017-07-05T00:27:07Z

src/ddmd/expression.d

            ob.writestring(")");
            ob.writenl();
        }
+        if (global.params.makeDeps !is null)


Hmm CodeCov says this is tested:

I've added an import expression to the test file, this should cover these lines.

ibuclaw · 2017-07-05T04:33:13Z

Seems like gdc doesn't like this change:

FYI, gdc compiled test uses GC allocation, not bump-the-pointer. So this means that there is a genuine logic bug in the implementation.

ibuclaw · 2017-07-05T04:57:34Z

src/ddmd/mars.d

+                depBuffer.writeByte(' ');
+            }
+            auto depf = File(outpath);
+            depf.setbuffer(cast(void*)depBuffer.data, depBuffer.offset);


This is normally why there's a double free. File is freeing the buffer owned by OutBuffer. You need to tell depf._ref = 1.

Fixed. This is my first encounter with dmd's code, so sorry that I didn't notice the _ref field in File.

ibuclaw · 2017-07-05T05:01:18Z

src/ddmd/globals.h

    const char *hdr_ext;        // for D 'header' import files
    const char *json_ext;       // for JSON files
    const char *map_ext;        // for .map files
+    const char *makedep_ext;    // for make-style dependency files


All _ext fields should just go. There is no need for them.

ibuclaw · 2017-07-05T05:05:50Z

src/ddmd/mars.d

+                    if(global.params.makeDeps !is null)
+                        error(Loc(), "-M/-MD can only be provided once!");
+                    global.params.makeDeps = new Strings();
+                    break;


-MD writes deps to a file implicitly.

ibuclaw · 2017-07-05T05:14:24Z

src/ddmd/globals.d


+    const(char)* makeDepsFile;          // filename for make-style dependencies output
+    const(char)* makeDepsTarget;        // the generated make target name
+    Array!(const(char)*)* makeDeps;     // array of paths that the compiled module(s) depend on


I wonder if these are strictly needed, or we should start requesting that new driver-only fields be added privately in mars.d so as to not affect other backends.

The makeDeps field could be private, but the File and Target can be useful for other drivers to input their commandline options to dmdfe. I've basically followed what changes the -deps option that was already built into dmd introduced, so that's why I added these fields here.

MartinNowak

Thanks, interesting PR :).
While dmd's command line is weird, it doesn't become better by adopting switches with different conventions. Please make your additions consistent with existing switches.
Furthermore this apparently cannot generate multiple output deps files, we need sth. that also works with multiple source and output files, while also working with parallel compilation (can't write everything into one file).

MartinNowak · 2017-07-11T06:36:49Z

src/ddmd/mars.d

  -J=<directory>   look for string imports also in directory
  -L=<linkerflag>  pass linkerflag to link
  -lib             generate library rather than object files
+  -M               generate make-style dependency file, generates no other output files


Dmd already has -o- to turn off object file generation.

MartinNowak · 2017-07-11T06:39:10Z

src/ddmd/mars.d

+  -M               generate make-style dependency file, generates no other output files
+  -MD              generate make-style dependency file
+  -MF=<filename>   specifies the dependency output file
+  -MT=<name>       specifies the name of the target rule in the dependencies file


This differs quite a lot from dmd's other options, look at -H, -Hd, -Hf, -op, -D.
In particular you want to be able to specify an output folder with -Md and obey -op, and use -Mf instead of -MF.
Please make sure to not mixup dependency and deps file in the explanation.

Not sure how -MT could work for multiple object files in one compilation (not a typical make use-case though possible).

MartinNowak · 2017-07-11T06:45:03Z

src/ddmd/mars.d

+            const(char)* mtarget = global.params.makeDepsTarget;
+            if (!outpath)
+            {
+                outpath = FileName.forceExt((*global.params.objfiles)[0], "dep");


You're sure there are object files when we run with -o-?
Better generate your own paths.

MartinNowak · 2017-07-11T06:56:39Z

src/ddmd/mars.d

+                        global.params.makeDeps = new Strings();
+                    break;
+                case 'F':
+                    if(p[3] != '=' || p[4] == '\0')


We do not use = for any of the path arguments, just -Hffile.di for example.

MartinNowak · 2017-07-11T06:58:25Z

test/compilable/extra-files/makedeps2.sh

+grep  'makedeps.sh'  ${RESULTS_DIR}/compilable/makedeps2.dep || exit 1
+grep  'makedeps_a.d'  ${RESULTS_DIR}/compilable/makedeps2.dep || exit 1
+grep  'object.d'  ${RESULTS_DIR}/compilable/makedeps2.dep || exit 1
+grep  '__entrypoint'  ${RESULTS_DIR}/compilable/makedeps2.dep && exit 1


You might want to include colon (:) in your grep searches.

eigenraven · 2017-08-12T12:40:42Z

Sorry for the long delay in fixing my PR, I had a surgery and needed time to recover. I've pushed the fixes now:

Changed the cmdline args to match -D*: -M, -Mf, -Md; -Mttarget follows the convention, they also respect -op by generating paths using the same logic as the rest of the code (Module's setOutfile)
Added a test for the target itself with a semicolon in the shell script
The code now fully handles multiple input modules:
- if Mf is specified, outputs all dependencies to a single file
- if Mf is not specified, generates a new ".dep" file for each input file basing on -Md/-op/cwd
- if MtTARGET is specified, all the targets are replaced with TARGET: (it's also clang's behaviour)
Dependency searching now uses a string hash table instead of the previous O(n^2) search for identical matches
Rebased against master

eigenraven · 2017-08-29T17:41:34Z

I have rebased the PR against the current master branch.

ximion · 2017-09-29T21:28:25Z

Any news on this? It's kind of important to make splitbuilds work out of the box in Automake, Meson & Co.

eigenraven · 2017-10-20T13:20:03Z

Would it be possible for you to re-review this PR? @MartinNowak @ibuclaw

eigenraven · 2017-12-17T17:54:34Z

Rebased against master.

jacob-carlborg · 2018-08-04T18:29:50Z

src/dmd/globals.d

+    bool doMakeDeps;                    // generate make-style dependencies file
+    const(char)* makeDepsFile;          // filename for make-style dependencies output
+    const(char)* makeDepsDir;           // directory for make-style dependencies output
+    const(char)* makeDepsTarget;        // the make target name


All new public symbols should have Ddoc comments.

jacob-carlborg · 2018-08-04T18:31:28Z

src/dmd/globals.d

+    bool doMakeDeps;                    // generate make-style dependencies file
+    const(char)* makeDepsFile;          // filename for make-style dependencies output
+    const(char)* makeDepsDir;           // directory for make-style dependencies output
+    const(char)* makeDepsTarget;        // the make target name


Can these const(char)* fields be made into D strings, i.e. const(char)[]? On the C++ side an ABI compatible struct can be used.

As much as I'd like this to happen, I don't think it's fair to ask a contributor to break the current status quo.
Such a change should be done on its own.

jacob-carlborg · 2018-08-04T18:34:54Z

src/dmd/dmodule.d

    Dsymbols* decldefs;         // top level declarations for this Module

    Modules aimports;           // all imported modules
+    Strings astringImports;     // all string-imported files


Perhaps this can be an array of const(char)[] and an ABI compatible struct on the C++ side.

Same comment about the status quo. I'd like to replace the Array aliases as much as the next guy, but I don't think we should put that kind of weight on this PR.

I also think that this is not really in the scope of this PR, and I think a "standard" way to do it (perhaps with predefined structs for most common array typs) should be defined first and a refactor PR could be done with that in place.

Then it will never happen and at the same time we’re adding more code that needs to change later 😞

jacob-carlborg · 2018-08-04T18:37:37Z

src/dmd/mars.d

+            {
+                auto mpath = m.srcfile.toString();
+                // Skip modules already in the table, add self
+                if (moduleImportedPaths.insert(mpath.ptr, mpath.length, null) is null)


Please add an overload to insert that takes const(char)[] instead of a pointer and length.

jacob-carlborg · 2018-08-04T18:53:42Z

src/dmd/root/filename.d

        return str;
    }
+
+    extern (D) const(char)[] toString() const pure


Can be made nothrow.

jacob-carlborg · 2018-08-04T18:55:55Z

src/dmd/root/file.d

+    extern (D) const(char)[] toString() pure const
+    {
+        return name.toString();
+    }


Can be made nothrow.

jacob-carlborg · 2018-08-04T18:56:13Z

src/dmd/root/file.d

-    extern (C++) const(char)* toChars() pure
+    extern (C++) const(char)* toChars() pure const
    {
        return name.toChars();


Can be made nothrow, I assume.

jacob-carlborg · 2018-08-04T18:57:27Z

src/dmd/mars.d

+                {
+                    // no actual file is created, uses just the path
+                    File* objfile = m.setOutfile(global.params.objname, global.params.objdir, m.arg, global.obj_ext);
+                    escapePathSpaces(&mdout, objfile.toChars());


Please used the newly added toString method instead.

Geod24 · 2018-07-30T05:15:29Z

src/dmd/mars.d

    // So deps file generation should be moved after the inlinig stage.
-    if (global.params.moduleDeps)
+    // inlineScan incrementally run semantic3 of each expanded functions.
+    // So deps file generation should be moved after the inlinig stage.


Duplicated comment

Geod24 · 2018-07-30T05:18:53Z

src/dmd/mars.d

+            {
+                const(char)* mpath = m.srcfile.toChars();
+                // Skip modules already in the table, add self
+                if(moduleImportedPaths.insert(mpath, strlen(mpath), null) is null)


Geod24 · 2018-07-30T05:21:28Z

src/dmd/utils.d

+ *   buf = Buffer to write the escaped path to
+ *   fname = Path to escape
+ */
+void escapePathSpaces(OutBuffer* buf, const(char)* fname)


Any chance to move this to filename ?

Geod24 · 2018-08-06T00:45:40Z

src/dmd/globals.d

+    bool doMakeDeps;                    // generate make-style dependencies file
+    const(char)* makeDepsFile;          // filename for make-style dependencies output
+    const(char)* makeDepsDir;           // directory for make-style dependencies output
+    const(char)* makeDepsTarget;        // the make target name


As much as I'd like this to happen, I don't think it's fair to ask a contributor to break the current status quo.
Such a change should be done on its own.

Geod24 · 2018-08-06T00:47:31Z

src/dmd/dmodule.d

    Dsymbols* decldefs;         // top level declarations for this Module

    Modules aimports;           // all imported modules
+    Strings astringImports;     // all string-imported files


Same comment about the status quo. I'd like to replace the Array aliases as much as the next guy, but I don't think we should put that kind of weight on this PR.

atilaneves · 2018-08-08T17:57:34Z

This PR would also close the issue I filed.

On the whole "why makefile dependencies?" front, I'd like to add this: this is required for ninja to work. Reggae only supports ninja by wrapping dmd and offering the necessary functionality.

WalterBright · 2018-08-08T20:19:52Z

I thought I'd added a comment here that dmd has the -deps switch which was supposed to emit makefile dependencies. I don't know when it got driven off into a ditch of uselessness.

eigenraven · 2018-08-08T20:43:06Z

I think it is too late now to change the behavior of the -deps switch, quite a few tools seem to rely on its format :(

WalterBright · 2018-08-08T23:47:10Z

src/dmd/root/filename.d

+
+    extern (D) const(char)[] toString() const pure nothrow
+    {
+        return str[0 .. strlen(str)];


str can be null

see #8550, which should be a separate PR anyway.

JinShil · 2018-08-09T06:49:10Z

So if this is merged, we'll have -deps, -X/-Xf, and then -M/-Mffile/-Mddirectory/-Mttarget all doing similar things in a different way? Can this herd of cats be reigned in somehow?

andralex · 2018-08-09T14:05:02Z

The right design is to output the information in a simple format and leave it to tooling to translate to others. Sadly, one little uninspired idea after another, we're here.

WalterBright · 2018-08-09T20:16:31Z

And so we arrive at the nightmarish blizzard of switches I so wanted to avoid :-(

ximion · 2018-08-09T20:52:04Z

Well, at some well-announced point in the future, you could in theory clean up the compiler flags and adjust them to something that you like.
Personally, I always prefer when a compiler copies the gcc CLI - not because it's particularly great, but because it is very complete and familiar, so achieving the same result with the same flags on different compilers is easy.
Anything goes, though.
The one thing I really want to avoid is having some extra binary to convert depfiles into multiple formats - that's not performant at all and will slow down compilation a lot due to the sheer number of process spawns.

rainers · 2018-08-14T07:00:54Z

src/dmd/mars.d

-    // So deps file generation should be moved after the inlinig stage.
-    if (global.params.moduleDeps)
+    // So deps file generation should be moved after the inlining stage.
+    if (global.params.moduleDeps || global.params.doMakeDeps)


You probably don't want to reuse -deps functionality as it is no longer useful for make. Analysing code via semantic3OnDependencies also reports local imports that are not used when compiling the code without the -deps option. It can also slow down compilation considerably. See discussion in #6748.

That was insightful, thank you. I've moved the semantic3OnDependencies call to be done only for the old -deps switch.

GoaLitiuM · 2018-12-10T22:05:04Z

What happened to this PR? Can we get this rebased and back to track again? I really wanted to see this go through to improve the building process with ninja.

MartinNowak · 2018-12-23T01:17:59Z

It's not in a particularly good shape, adds lots of extra switches with partly redundant functionality (#6961 (comment)).
More importantly recursive import dependencies are not what you want for make et.al.
Those tools know well how to stich together shallow dependencies themselves. Also recursive dependencies can only be reliably generated by the compiler when actually compiling a dependency (e.g. local imports in templates).

MartinNowak · 2018-12-23T01:23:14Z

The basic idea to generate makefile compatible output is good though, maybe just add a -makedeps=<filename> switch, or given that -deps was broken since a year (#9122), maybe you could just change the format of -deps=<filename>. Another approach would be to switch the output format based on the file-ending, though makefile dependencies often use .d which might be somewhat confusing.

Geod24 · 2020-12-29T11:33:41Z

Another implementation, #12011, has been merged into master.

ximion · 2020-12-29T16:56:59Z

Excellent, many thanks to everyone involved! This helps a lot for project with mixed languages and D with buildsystems using ninja and make :-)

dlang-bot added the Severity:Enhancement label Jul 3, 2017

wilzbach reviewed Jul 5, 2017

View reviewed changes

ibuclaw requested changes Jul 5, 2017

View reviewed changes

eigenraven force-pushed the makedepends branch 2 times, most recently from 9303ba1 to 5a5ba85 Compare July 5, 2017 10:56

MartinNowak requested changes Jul 11, 2017

View reviewed changes

eigenraven force-pushed the makedepends branch from 5a5ba85 to b34bb82 Compare August 12, 2017 12:31

eigenraven force-pushed the makedepends branch 2 times, most recently from fdb975b to 62a9086 Compare August 12, 2017 13:19

eigenraven force-pushed the makedepends branch from 62a9086 to 9f597ca Compare August 29, 2017 10:18

ximion mentioned this pull request Oct 3, 2017

Add Meson build definition s-ludwig/ddmp#5

Merged

ximion mentioned this pull request Dec 3, 2017

Please output Makefile-style depfiles for ninja and make ldc-developers/ldc#1802

Closed

eigenraven force-pushed the makedepends branch from 9f597ca to 35581f5 Compare December 17, 2017 17:52

eigenraven requested a review from WalterBright as a code owner December 17, 2017 17:52

dlang-bot added Review:Needs Rebase and removed Review:Needs Rebase labels Jan 1, 2018

jacob-carlborg suggested changes Aug 4, 2018

View reviewed changes

jacob-carlborg reviewed Aug 4, 2018

View reviewed changes

src/dmd/root/filename.d Outdated

return str;

}

extern (D) const(char)[] toString() const pure

Copy link

Contributor

jacob-carlborg Aug 4, 2018

Choose a reason for hiding this comment

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

Can be made nothrow.

jacob-carlborg reviewed Aug 4, 2018

View reviewed changes

src/dmd/root/file.d

extern (D) const(char)[] toString() pure const

{

return name.toString();

}

Copy link

Contributor

jacob-carlborg Aug 4, 2018

Choose a reason for hiding this comment

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

Can be made nothrow.

jacob-carlborg reviewed Aug 4, 2018

View reviewed changes

Geod24 reviewed Aug 6, 2018

View reviewed changes

eigenraven force-pushed the makedepends branch from 5431fb9 to 995fa60 Compare August 8, 2018 20:37

WalterBright reviewed Aug 8, 2018

View reviewed changes

rainers reviewed Aug 14, 2018

View reviewed changes

eigenraven force-pushed the makedepends branch from 995fa60 to 001608b Compare August 23, 2018 21:15

fix issue 16746 : Add make-style dependency file generation (-M*)

7556d76

eigenraven force-pushed the makedepends branch from 001608b to 7556d76 Compare August 23, 2018 21:17

ximion mentioned this pull request Aug 27, 2018

Move to Meson for build system gnunn1/tilix#1502

Closed

wilzbach mentioned this pull request Jul 1, 2020

Fix issue 14188 - implement emitting Makefile-compatible dependencies file #11357

Closed

rtbo mentioned this pull request Nov 30, 2020

Fix issue 14188 - implement makefile compatible dependency file #12011

Merged

Geod24 closed this Dec 29, 2020

Uh oh!

Conversation

eigenraven commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eigenraven commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eigenraven commented Aug 29, 2017

Uh oh!

ximion commented Sep 29, 2017

Uh oh!

eigenraven commented Oct 20, 2017

Uh oh!

eigenraven commented Dec 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eigenraven commented Jul 3, 2017 •

edited

Loading

dlang-bot commented Jul 3, 2017 •

edited

Loading

eigenraven commented Aug 12, 2017 •

edited

Loading