-
-
Notifications
You must be signed in to change notification settings - Fork 670
Fix dependency chains in posix.mak #6586
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
Fix dependency chains in posix.mak #6586
Conversation
78c7df7 to
7f1962c
Compare
|
@andralex while I really appreciate the fast review and intend to merge I think we shouldn't overrun the usual one-day "cry loud if you can" waiting period, especially as this is non-trivial and there were some issues with CircleCi. |
| CC=$(HOST_CXX) $(HOST_DMD_RUN) -of$@ $(MODEL_FLAG) -vtls -J$G -J../res -L-lstdc++ $(DFLAGS) $(filter-out $(STRING_IMPORT_FILES) $(HOST_DMD_PATH),$^) -version=NoBackend | ||
|
|
||
| dmd: $G/dmd | ||
| dmd: $G/dmd $G/dmd.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be the biggest proposed change but I couldn't see any reason why we shouldn't auto-generate the dmd.conf into generated during a normal build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should, but it also needs to be copied to src/dmd.conf while we're still using that.
Also the target for make -C src -f posix.mak dmd.conf (src/posix.mak) is used in many places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should, but it also needs to be copied to src/dmd.conf while we're still using that.
Well, we can't copy it because the relative paths are different, but I put the "old" dmd.conf target back in.
Once @RazvanN7 does the ddmd -> dmd transition, he should be able to remove this legacy target :)
Also the target for
make -C src -f posix.mak dmd.conf(src/posix.mak) is used in many places.
Yup I noticed - the Travis and CircleCi scripts have this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One reason this wasn't done previously is that a dmd.conf in the current working dir applies to any dmd being run, it has the highest precedence in the config search order. Super-annoying IMO, but I won't argue any more.
Since we've switched to explicit -conf= it shouldn't cause much troubles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have won the fight against CircleCi & Travis :) |
e481124 to
1ef7163
Compare
|
How do you generate the |
|
Except for complex PR reviews the value of adding new changes as tiny commits and later squashing seems very small compare to all the extra decisions/complications it adds. |
Manually. I though it doesn't matter, because with auto-merge-squash all commits will be squashed into one anyways.
Well again, the idea is that one doesn't have to look at the entire diff if only one line changed and again with |
generatedthis isn't the case anymore (at the moment everybuildis a full rebuilddmd.confneeds to be put into thegeneratedfolder as wellFor more infos on (4) see this guide or this SO question.
CC @RazvanN7