-
-
Notifications
You must be signed in to change notification settings - Fork 672
Fix windows debug build #7431
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 windows debug build #7431
Conversation
|
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:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
6aeebf0 to
4f37630
Compare
wilzbach
left a comment
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'm not sure why main was removed in the first place, maybe there was a reason so if there was please chime in let me know if another kind of change is needed here.
The main method can't be part of the dmd unittest executable. This is a well-known limitation of D's unittest, but luckily this will be fixed in 2.080. The correct fix is to remove -unittest from the DDEBUG flags in win32.mak:
Line 135 in 7c888c4
| DDEBUG=-debug -g -unittest |
After #6767, I actually tried to add a similar target to win32.mak, but I'm still fighting with the auto-tester / Windows build setup: #7414
4f37630 to
7785e7a
Compare
7785e7a to
daef706
Compare
|
So that means you cannot compile in unittests into your dmd executable on windows, you have to make a seperate exe for your unittests. I've updated this PR with another change that "works around" this issue of not being able to include the main in "mars.d" without preventing a user from being able to include unittests in the main build. |
Why would you want to run all unittest whenever you call the DMD executable? What's the use case? |
|
So I don't have to build dmd twice when I'm in development. |
|
Come to think of it, a nice feature for dmd would be to build in unittests, but have a way to turn them on and off whenever your run your exe. |
That's quite a atypical way to do debugging (at least for a Linux dev) 😱 FWIW if you intend to use this strategy, you will need to pass
|
|
There's pros and cons to each method. In any case, if you can support both use cases why wouldn't you? If a developer wants to add the "-unittest" flag to their DEBUG build, the current code would cause a very cryptic set of linker errors. This of course should be solved by fixing the linker but it's been that way forever with no solution in sight. Those druntime options look real nice, adding a |
NoMain seems to be an compromise between both worlds.
|
With the current code you get the following With this PR you get this The second set of options is more intuitive. Then if the developer realizes they need to disable "main" from mars.d, they'll browse to the code so they can "comment it out" and see there is already a |
I don't think there are Windows specific approaches to testing. Building both test and main into the same excutable and running them both is just the (rather unconventional) way that has been more or less enforced by dmd for years. |
wilzbach
left a comment
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'm sorry for breaking your workflow. I didn't really consider that people might be abusing the -unittest feature. As mentioned above, you won't be able to use this approach after 2.080 without modifications, but that's a battle for a different day and I still have hopes that until then you will realize the advantages of having a separate test executable ;-)
Also we will hopefully get more and more unittest blocks within the DMD codebase, because it's now a proper Makefile target on Posix and actively checked by the auto-tester, but sticking with the status quo and using the NoMain versioning sounds reasonable and should as you said please everyone.
So people under Windows currently don't/can't build a DMD executable with debug symbols and without unittests under Windows? |
|
I think the idea behind the initial design of |
I think he was just saying that by default, the debug build also includes unittests. There's no reason a developer couldn't remove that flag in the makefile.
I'm not familiar with the change coming in to 2.080, are we changing the semantics of the |
Yes of course, but I was asking out-of-the-box? Needing to modify the Makefile isn't a good practice.
Well, the default behavior of how |
Windows debug build is currently broken because dmd does not generate the 'main' function. I bisected the cause to this commit (fed25ec). It looks like the
mainmethod was removed in the-unittestbuild which explains what happened. I've removed that change so that themainmethod still exists in a unittest build. I'm not sure why main was removed in the first place, maybe there was a reason so if there was please chime in let me know if another kind of change is needed here.