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.

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

Make rdmd compile and behave correctly again #10

Closed
wants to merge 3 commits into from

Conversation

kyllingstad
Copy link
Member

Before these commits are merged, they should be reviewed by someone with intimate knowledge of the inner workings of rdmd.

For some reason, the "binary" (the name of the dmd executable) and "config"
(the path to dmd.conf) lines from the .deps file were included as
dependencies.  This commit makes rdmd ignore these lines.
@kyllingstad
Copy link
Member Author

@ghost
Copy link

ghost commented Aug 19, 2011

It seems this was caused by pull #6. #6

@Abscissa
Copy link
Contributor

Abscissa commented Sep 1, 2011

"In the .rsp file, which is supposed to contain DMD's command line
parameters (when there are many of them), the string
"dmd /path/to/dmd.conf" now appears.

I've traced this back to rdmd.d line 414-437, in which the
"binary" (i.e. dmd) and "config" (i.e. dmd.conf) lines of the .deps file
get added as dependencies of the program being compiled. In other
words, it appears to be very much intentional. I have no idea why,
though."

The "binary" and "config" were added because without them, RDMD silently fails to recompile apps when:

  1. A different DMD is used.
    or
  2. sc.ini/dmd.conf is changed.

Those situations should trigger a rebuild, since if either of those are changed, then obviously the intent is to actually use the changes.

(Changed libraries should also trigger a rebuild, but that's a much more difficult issue to solve so I left it out for now.)

Adding "/path/to/dmd.conf" to the cmdline/rsp file was an unintended side-effect.

This pull request could be acceptable as a temporary solution if we want to push out a new release right away. But the proper solution is to keep "binary" and "config" and just change it so they don't get added to the command line or rsp file.

I don't know what the hell happened with that one remaining use of "myModules". I know I successfully compiled the changes I made in that pull request (#6). Maybe git screwed up and reverted that part during one of the merges with upstream.

@Abscissa
Copy link
Contributor

Abscissa commented Sep 1, 2011

(Gah...GitHub keeps screwing around with my formatting...)

@NilsBossung
Copy link
Contributor

Abscissa, it seems you had done some filtering, but it was lost in the conflict between our changes. So maybe it's enough to put that back in there?

@Abscissa
Copy link
Contributor

Abscissa commented Sep 1, 2011

Ahh, yes, you're right. Seems that's also how "myModules" got snuck back in. All the issues in this pull request #10 (except for the 6524 workaround) were the result of a single botched merge: Abscissa@73e2fc6#L0L315

I'll have to be more careful with my manual merges.

Lars, how do you want to handle this? Do you want to add that filtering (that was removed in the link above) back in inside this pull request, or would you rather revert this pull request's last two commits (leaving just the 6524 workaround) and then I'll create a new pull request fixing the issues from my botched merge?

@Abscissa
Copy link
Contributor

Abscissa commented Sep 2, 2011

Ok, kyllingstad, if you revert your second and third commits (0fbe489 and cae99e2), then you can pull the two commits I made here (Or I can just make it a new pull request if you'd rather):

https://github.com/Abscissa/tools/commits/master

The first commit fixes everything I screwed up in that manual merge I did (ie, the "myModule", and the dmd.conf/etc working their way into the response file).

The second commit fixes a couple problems I ran into trying to compile RDMD with the latest GitHub DMD/Phobos (ie, isdir and std.regexp are now deprecated in favor of isDir and std.regex). Someone who's better familiar with std.regex should check that commit though, I wasn't able to test it because...

There's still a problem compiling this with both 2.054 and the latest GitHub: I keep getting this:

rdmd.d(456): Error: pure function 'shellQuote' cannot call impure function 'replace'

Looks like its a complication from your 6524 workaround.

@Abscissa
Copy link
Contributor

Abscissa commented Sep 2, 2011

Ahh, I see, your workaround probably works on Posix. But on Windows, the shellQuote function uses std.array.replace which is apprently impure. The Posix version of shellQuote doesn't use replace.

@Abscissa
Copy link
Contributor

Abscissa commented Sep 2, 2011

Looking into it more, issue 6524 (or rather, 3180) is a hugely problematic regression (I've run into it before myself) and I, for one (for what little that counts ;) ), REALLY don't think there should be a new release before the fix for it gets merged:

dlang/dmd#96

So unless there's another really easy workaround, I think we should just make sure Walter's willing to get that merged before the next beta (which sounds like it should be very soon, going by the chatter on the beta list).

@Abscissa
Copy link
Contributor

Abscissa commented Sep 2, 2011

I've fixed the workaround to work on windows, too, and created a new pull request for the whole thing: #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants