Skip to content

(Option 1) Cause win32.mak to print error when HOST_DC is not set#7467

Merged
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:fix_windows_mak
Jan 6, 2018
Merged

(Option 1) Cause win32.mak to print error when HOST_DC is not set#7467
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:fix_windows_mak

Conversation

@marler8997
Copy link
Copy Markdown
Contributor

I spent way too much time on this one. The limitations of Digital Mars' make program made this one quite difficult, this is the best mechanism I could come up with to cause the make file to fail with a nice error messages when HOST_DC is not set.

@dlang-bot
Copy link
Copy Markdown
Contributor

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.

@JinShil
Copy link
Copy Markdown
Contributor

JinShil commented Dec 19, 2017

See also #7256

@marler8997 marler8997 changed the title Cause win32.mak to print error when HOST_DC is not set (Option 1) Cause win32.mak to print error when HOST_DC is not set Dec 19, 2017
@marler8997 marler8997 force-pushed the fix_windows_mak branch 5 times, most recently from 853a8af to 349a34d Compare December 19, 2017 17:23
@marler8997
Copy link
Copy Markdown
Contributor Author

Had to make another change, it appears to be another bug in digital mars make, I had to split the debdmd and reldmd targets into one with their dependencies and one with their rules/actions. If both the dependencies and actions are in the same staement, then make will not execute the actions.

Copy link
Copy Markdown
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

I'm unclear about how this works. I'd expect it would be simpler, what's with all the additional work? What does "deb" and "rel" mean? I assume "debug" and "release"? It was confusing as heck because "deb" also stands in for "debian".

@@ -288,10 +288,18 @@ release:
$G :
if not exist "$G" mkdir $G
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has never worked well for me. I have no idea how it works for other people. Apparently the approach has issues: https://stackoverflow.com/questions/4440500/depending-on-directories-in-make

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe the stack overflow question you submitted is an issue with GNU make. This makefile uses Digital Mars make and I haven't seen an issue with how it handles directory dependencies.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@andralex as I have stated on other pull requests in the past: depending on directories works only if you use them as order-only dependencies.
Anyhow DigitalMars Make so different from Make, that it shouldn't be called Make 😉

@marler8997
Copy link
Copy Markdown
Contributor Author

I'd expect it to be simpler as well, but this was the simplest I could figure out how to do. I believe my changes along with the existing complexity in the Makefile is due to limitations and bugs in Digital Mars make.

@JinShil
Copy link
Copy Markdown
Contributor

JinShil commented Dec 20, 2017

What does "deb" and "rel" mean?

Indeed. Please spell things out.

@marler8997
Copy link
Copy Markdown
Contributor Author

marler8997 commented Dec 20, 2017

debdmd and reldmd are public targets apart of the public interface of the Makefile. I'm not against changing them but I would consider those changes outside of the scope of this PR. This PR only addresses proper error handling of the HOST_DC environment variable.

@marler8997 marler8997 force-pushed the fix_windows_mak branch 3 times, most recently from a3ef9ec to 148163a Compare December 20, 2017 15:33
Copy link
Copy Markdown
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I prefer this approach over Option 2, it's a bit more DRY.

src/win32.mak Outdated
debdmd: check-host-dc debdmd-make

debdmd-make:
@echo reldmd
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this line needed?

Copy link
Copy Markdown
Contributor Author

@marler8997 marler8997 Dec 21, 2017

Choose a reason for hiding this comment

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

I assume you are asking why have the extra target, why not do this:

debdmd: check-host-dc 
    $(DMDMAKE) "OPT=" "DEBUG=-D -g -DUNITTEST" "DDEBUG=-debug -g -unittest" "DOPT=" "LFLAGS=-L/ma/co/la" $(TARGETEXE)

Instead of this:

debdmd: check-host-dc debdmd-make

debdmd-make:
    $(DMDMAKE) "OPT=" "DEBUG=-D -g -DUNITTEST" "DDEBUG=-debug -g -unittest" "DOPT=" "LFLAGS=-L/ma/co/la" $(TARGETEXE)

The answer is because it doesn't work :) I don't know why, my guess is it's a bug in Digital Mars Make, but if you combine them into 1 statement, then make ignores the commands.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I was trying to politely ask you to remove the debugging output ;-)
I understand that DigitalMars Make is "rather rudimentary" ...

@marler8997 marler8997 force-pushed the fix_windows_mak branch 2 times, most recently from 6ce9b82 to cb482c6 Compare December 21, 2017 17:59
@dlang-bot dlang-bot merged commit cd9b8b9 into dlang:master Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants