Fix rdmd --makedep(end|file) (issues 12351 and 12354) #122

leandro-lucarella-sociomantic · 2014-03-12T14:26:21Z

This pull request basically makes rdmd --makedep(end|file) useful. I don't know if anybody used it before (checking it does work), because I have no idea how it could have worked.

Basically this fixes 2 important issues (use the binary produced as the target instead of the source file, and list all the dependencies as empty targets to avoid make errors if you remove a file). Also the output format is improved to make it both more testeable and human-readable.

leandro-lucarella-sociomantic · 2014-03-12T18:11:34Z

rdmd.d

-    // --build-only implies the user would like a binary in the program's directory
-    if (buildOnly && !exe)
+    // If there is no -of the user would like a binary in the program's directory
+    if (!exe)


I would appreciate if someone can take a look at this bit in particular. I hope is true and the buildOnly check was only there for historical reasons.

No this isn't right, rdmd should not pollute the current directory with executables if -of isn't used, it should use a temp folder instead.

@andralex: requesting confirmation on this.

No, you are right, is leaving the binary there with this change even if -of is not used.

Will fix, but I don't think the fix will be particularly pretty :S

BTW, the rdmd test program is not checking this :P

If there is no -of the user would like a binary in the program's directory

Absolutely, this is wrong. If there is no -of and no --build-only, the user just wants to run the program, and rdmd needs to build to the temporary directory.

The test suite should probably check that rdmd isn't leaving any junk in the program's directory, unless the user asked for it.

leandro-lucarella-sociomantic · 2014-03-12T22:08:55Z

I just added 2 more commits to definitely fix this feature. I updated the PR title and description.

I'll take a look at what you say, but I had the impression the binary is not moved even with this change. I'll confirm.

CyberShadow · 2014-03-12T22:15:52Z

Does --makedepend even make sense without -of / -od? I think it should imply it.

leandro-lucarella-sociomantic · 2014-03-12T22:29:38Z

How can you imply -of? You mean behave the same as when --build-only is specified? This is what I'm doing.

leandro-lucarella-sociomantic · 2014-03-12T22:54:24Z

OK, new commit pushed, all my tests look good now.

CyberShadow · 2014-03-12T23:03:08Z

rdmd.d

-    // --build-only implies the user would like a binary in the program's directory
-    if (buildOnly && !exe)
-        exe = exeDirname ~ dirSeparator;
+    makeDepTarget = exe;


Can you restructure the code so the new behavior is more encapsulated? --makedep* isn't part of the usual rdmd operation, the code is a bit difficult to understand as it is

As I just said, maybe is more sane to just require -of when --makedep* is used and forget about all this mess.

Requiring -of or -od sounds good to me. Even better than implying them, I think.

yeah, I'll do that, I'll require -of, if you want to create a makefile, you really need to specify the target name. Make is too sensitive to guess anything.

leandro-lucarella-sociomantic · 2014-03-13T05:28:47Z

Updated, let's see if this one is a keeper :D

CyberShadow · 2014-03-13T05:32:47Z

rdmd.d

@@ -139,6 +139,14 @@ int main(string[] args)
    if (bailout) return 0;
    if (dryRun) chatty = true; // dry-run implies chatty

+    if ((makeDepend || makeDepFile) && (!exe || exe.endsWith(dirSeparator)))


exe.endsWith(dirSeparator)

Why not reuse the existing logic which calculates exe if -od is set? I don't see any reason why something should work with only -of but not -od.

At lines 188-196

Just move the check inside writeDeps (e.g. enforce(exe, "Executable location is not specified");). The calls to it are conveniently located after the point where exe is calculated if -of or -od are set, but before the point where it is calculated if it's going to be in a temporary directory.

Why not reuse the existing logic which calculates exe if -od is set? I don't see any reason why something should work with only -of but not -od.

Is a simplification because I don't see is realistic to use -od with --makedep* and it greatly simplifies the code. I would keep it like this and if anybody needs it in the future it can be extended. I have the feeling nobody ever will (famous last words :P).

Just move the check inside writeDeps (e.g. enforce(exe, "Executable location is not specified");).

If I do this I have to take care again about the added paths to the target, which is really annoying for something probably nobody will ever use.

Are these cases in the test suite?

No. But here is an example. I know is a very bizarre corner case, but as I said before I stumbled to this problem in the past and is no fun to debug.

$ cd /tmp $ mkdir make $ cat <<EOT > Makefile ./../make/.././t: echo t > $@ ../t: blah EOT $ make ./../make/.././t echo t > ../make/.././t $ make ../t make: *** No rule to make target `blah', needed by `../t'. Stop.

So as you can see, ./../make/.././t and ../t targets are treated differently. If you have -od./../make/.././ ./../make/.././t.d and you normalize the path, you'll get ../t for the binary, but the source will be ./../make/.././t.d, so depending on how you call make ./../make/.././t or make ../t it will work or not.

I know it might work for 99.9% of the cases, but my point is it's still weak and provide absolutely 0 advantage (when you write a makefile you know how your target should be named and you should use -of).

Adding a convoluted case like this to the tests is overkill and so is trying to figure out a more sane case that also fails, this is why I'm not willing to spend a lot more time on this.

I would like to settle with something, rdmd right now is completely broken to calculate dependencies, this PR in any form (with or without accept -od) will bring it to usefulness (although I strongly believe accepting -od is the wrong approach).

Well, I just don't think this is a good argument, I mean you're pushing hard for a fix to a specific case but don't want to add a test to it, and the refactoring I suggested seems like an obvious one that any next guy who comes along might do and break your case.

Anyway, if you're OK with that, I guess I have no objections.

and the refactoring I suggested seems like an obvious one that any next guy who comes along might do and break your case.

This is a fair point, I can add a comment mentioning this and pointing to the discussion. Seriously, adding test cases for these kind of corner cases would be extremely time consuming, is killing a fly with a bazooka.

Was the plan to add an entire make file or something? I think just checking that rdmd isn't mangling the paths in unexpected ways would be enough, no?

Was the plan to add an entire make file or something?

I don't understand what do you mean by this.

I think just checking that rdmd isn't mangling the paths in unexpected ways would be enough, no?

Yes, but is not so simple, the problem is the input data for the test :)
And I'm really, really sorry but I don't have the time to dedicate to build a good rdmd test suite. I think I did a fair job by testing what I changed, but I think making a proper test suite for rdmd is beyond the scope of this PR.

CyberShadow · 2014-03-13T05:40:40Z

What happened to stripping the leading ./? I don't see where it is happening in the code now.

ghost · 2014-03-13T07:54:35Z

BTW, the rdmd test program is not checking this :P

Yeah, the test-suite was implemented fairly recently. RDMD has grown quite organically, it doesn't have a feature list except what the help screen says. :)

leandro-lucarella-sociomantic · 2014-03-13T13:30:23Z

What happened to stripping the leading ./? I don't see where it is happening in the code now.

If I require of this is not necessary anymore, that's the beautify of it :)

That is only necessary for target names synthesized by rdmd (which I don't think it makes sense at all for --makedep*).

leandro-lucarella-sociomantic · 2014-03-13T19:03:26Z

Updated adding a comment to clarify why we only support -of for now.

andralex · 2014-03-15T23:03:10Z

rdmd.d

@@ -139,6 +139,21 @@ int main(string[] args)
    if (bailout) return 0;
    if (dryRun) chatty = true; // dry-run implies chatty

+    /* Only -of is supported because Make is very suceptible to file names, and


susceptible

Thanks, fixed.

andralex · 2014-03-15T23:37:07Z

The way the feature worked was to list the main source file as depending on its own transitive imports. Then it was assumed the makefile would have a rule that made the exe dependent on the main source file. Like this:

acme : main.d
    rdmd --build-only -of$@ $<

# generated by rdmd --makedepend main.d
main.d : stuff.d morestuff.d evenmorestuff.d

Usually the generated part is included from a different file. With that setup:

If you change main.d you'd need to run rdmd --makedepend again.
If you change some dependency things work fine

Please confirm that by this diff the proposed setup is:

# generated by rdmd --makedepend -ofacme main.d
acme : main.d stuff.d morestuff.d evenmorestuff.d
    rdmd --build-only -of$@ main.d

With the proposed setup, things work more or less the same except there's no need to write the first line in the makefile by hand.

Is that correct?

leandro-lucarella-sociomantic · 2014-03-19T17:57:29Z

acme : main.d
    rdmd --build-only -of$@ $<

# generated by rdmd --makedepend main.d
main.d : stuff.d morestuff.d evenmorestuff.d

I'm sorry to break it to you but this makefile is broken. Just try it. build it, then touch one of the dependencies (without touching main.d) and you'll see nothing is rebuilt. The problem is main.d is a file, and by using main.d: stuff.d morestuff.d evenmorestuff.d, you are telling Make what is needed to build main.d, not about transitive dependencies. So if main.d is already built (since is a source file it will ever be). So what happens is, if stuff.d is newer than main.d, then main.d needs to be triggered but since main.d is not really rebuilt, then nothing happens, because main.d will still be older than acme. For that to work you should do something like:

main.d:
    touch $@

Which is clearly a hack and changes the timestamp of main.d when there is no need to.

With this change, if you already used --makedepend, all you need to do is pass the correct binary target name to rdmd, nothing else should change. I don't know how do you usually generate the dependencies file, so I'll assume, as you did, they are already created). Here is a proposed setup for your example:

acme: main.d
    rdmd --build-only -of$@ main.d

You could replace main.d in the action with $(firstword $(filter %.d,$^)) if you want a more generic rule, that should work if you always put the source with the main as the first dependency (the generate file does, but depends which one Make sees first).

I would strongly recommend to use --makedepfile instead though, to take care of both in one step:

acme : main.d
    rdmd --build-only --makedepfile deps -of$@ $(firstword $(filter %.d,$^))

-include deps

… target Having the source file as the target for the dependencies on a Makefile is useless, as there is no rule to rebuild the source file, Make can't use that information at all. To a have a meaningful target name, now --makedep* options require -of to be present too.

…for all source files Now rdmd --makedep(end|file) will print an empty rule for each source file.

The current format is not very human friendly and also makes doing simple checks for testing quite hard. The new format is based on GCC's format, and it looks like this: target: \ source1.d \ source2.d \ source1.d: source2.d:

andralex · 2014-03-19T19:36:45Z

you're right, thanks for the explanation

…arget Fix rdmd --makedep(end|file) (issues 12351 and 12354)

leandro-lucarella-sociomantic reviewed Mar 12, 2014
View reviewed changes

CyberShadow reviewed Mar 12, 2014
View reviewed changes

CyberShadow reviewed Mar 13, 2014
View reviewed changes

andralex reviewed Mar 15, 2014
View reviewed changes

leandro-lucarella-sociomantic added 3 commits March 19, 2014 19:43

Fix issue 12354: rdmd --makedep(end|file) doesn't output empty rules …

314d92e

…for all source files Now rdmd --makedep(end|file) will print an empty rule for each source file.

rdmd: Improve --makedep(end|file) output format

07553e1

The current format is not very human friendly and also makes doing simple checks for testing quite hard. The new format is based on GCC's format, and it looks like this: target: \ source1.d \ source2.d \ source1.d: source2.d:

andralex added a commit that referenced this pull request Mar 19, 2014

Merge pull request #122 from leandro-lucarella-sociomantic/rdmd-exe-t…

09aaa09

…arget Fix rdmd --makedep(end|file) (issues 12351 and 12354)

andralex merged commit 09aaa09 into dlang:master Mar 19, 2014

leandro-lucarella-sociomantic deleted the rdmd-exe-target branch March 19, 2014 20:57

This was referenced Jan 18, 2018

rdmd --makedep(end|file) uses the source file as the target wilzbach/tools-test#104

Closed

rdmd --makedep(end|file) doesn't output empty rules for all source files wilzbach/tools-test#105

Closed

Fix rdmd --makedep(end|file) (issues 12351 and 12354) #122

Fix rdmd --makedep(end|file) (issues 12351 and 12354) #122

Conversation

leandro-lucarella-sociomantic commented Mar 12, 2014

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandro-lucarella-sociomantic commented Mar 12, 2014

CyberShadow commented Mar 12, 2014

leandro-lucarella-sociomantic commented Mar 12, 2014

leandro-lucarella-sociomantic commented Mar 12, 2014

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

leandro-lucarella-sociomantic commented Mar 13, 2014

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

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

CyberShadow commented Mar 13, 2014

ghost commented Mar 13, 2014

leandro-lucarella-sociomantic commented Mar 13, 2014

leandro-lucarella-sociomantic commented Mar 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex commented Mar 15, 2014

leandro-lucarella-sociomantic commented Mar 19, 2014

andralex commented Mar 19, 2014