Run dmd internal unittests on CIs by wilzbach · Pull Request #6767 · dlang/dmd

wilzbach · 2017-05-10T11:10:49Z

This will probably take a bit to configure for all CI test systems - especially for the auto-tester...

andralex · 2017-11-17T18:07:16Z

works?

dlang-bot · 2017-11-28T18:23:00Z

Thanks for your pull request, @wilzbach!

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.

wilzbach · 2017-11-28T18:26:19Z

works?

Yes, it should work now. As the PR title states this allows to build all unittest blocks within the dmd codebase and runs them on our CI systems (CircleCi + auto-tester).
Also, the Makefile target can be invoked manually:

make -f posix.mak test

I will add a similar target for the Windows Makefiles once this has been approved.

andralex

awesome! now we need unittests :)

andralex · 2017-11-29T04:44:53Z

src/posix.mak

+$G/dmd-unittest: $(DMD_SRCS) $(ROOT_SRCS) $G/newdelete.o $G/lexer.a $(G_GLUE_OBJS) $(G_OBJS) $(STRING_IMPORT_FILES) $(HOST_DMD_PATH)
+	CC=$(HOST_CXX) $(HOST_DMD_RUN) -of$@ $(MODEL_FLAG) -vtls -J$G -J../res -L-lstdc++ $(DFLAGS) -unittest -main $(filter-out $(STRING_IMPORT_FILES) $(HOST_DMD_PATH),$^)
+
+test: $G/dmd-unittest


could you please call this target 'unittest'? It has that name in druntime and phobos.

braddr · 2017-11-29T06:31:09Z

This change is a good direction, but misses a few details about the current system. There's a test target at the top level that invokes the deeper level directories as needed. There's also a separation between build and test. I don't think having the src level's auto-tester-build step invoking tests is correct. The auto-tester-test target ought to do that. Right now, the top level auto-tester-test target just invokes the test directories makefile. Adding a line to have it invoke the src/posix.mak unittests would fit the current scheme better.

If you want to get fancy, the build steps could produce the dmd-unittest binary, but let the test steps invoke it, but that doesn't match how the other tests are treated.

wilzbach · 2017-12-01T04:49:58Z

posix.mak


 all:
-	$(QUIET)$(MAKE) -C src -f posix.mak
+	$(QUIET)$(MAKE) -C src -f posix.mak all


This makes the default Makefile target explicit (all is the default target of src/posix.mak).

Is there a disadvantage to assume the default target builds all? After all, if it's not it means we wrote that makefile poorly.

Explicitly for the win - it was helpful during debugging this.

wilzbach · 2017-12-05T04:13:28Z

Okay so the current status is that CircleCi works nicely again, but I am having troubles with the auto-tester as no HOST_DC is set on the "test dmd" step:

HOST_DC=/home/ec2-user/sandbox/at-client/release-build/install/linux/bin32/dmd
make -C src -f posix.mak unittest
no cpu specified, assuming X86
which: no dmd in (/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin:/opt/aws/bin)
make[1]: Entering directory `/home/ec2-user/sandbox/at-client/pull-2926855-Linux_32_64/dmd/src'
posix.mak:134: *** 'dmd' not found, get a D compiler or make AUTO_BOOTSTRAP=1.  Stop.
make[1]: Leaving directory `/home/ec2-user/sandbox/at-client/pull-2926855-Linux_32_64/dmd/src'
make: *** [test] Error 2

I tried to hack around this, but exposing HOST_DC would be very helpful.
I opened a PR at the auto-tester -> braddr/at-client#6

wilzbach · 2017-12-06T06:12:43Z

Hmm now that I reverted the hack to detect the already built D compiler and instead I was hoping that HOST_DC is now properly exported and available as environment variable, but sadly that doesn't seem to be the case:

https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=2928984&isPull=true

Any ideas on what I could have missed? Should I go with the fallback hack which detects the already built D compiler for now?

braddr · 2017-12-07T00:44:06Z

You hit some of the 3 build hosts that hadn't yet updated with the changes. I took all of them out of the active fleet until they can be updated (I don't have remote access to them). Slows the fleet down, but allows the tests to successfully build this change.

wilzbach · 2017-12-07T05:05:19Z

You hit some of the 3 build hosts that hadn't yet updated with the changes. I took all of them out of the active fleet until they can be updated (I don't have remote access to them). Slows the fleet down, but allows the tests to successfully build this change.

Awesome. Thanks a lot!
Then this should be ready to go 🎉
CC @andralex @CyberShadow

PetarKirov · 2017-12-07T07:55:57Z

posix.mak

 	$(RM) tags

 test:
+	$(QUIET)$(MAKE) -C src -f posix.mak unittest


It looks like you don't have to modify src/posix.mak, if you simply replace here unittest with ENABLE_DEBUG=1. For reference, ENABLE_DEBUG does the following:

ifdef ENABLE_DEBUG CXXFLAGS += -g -g3 -DDEBUG=1 -DUNITTEST DFLAGS += -g -debug -unittest

I think going with this option, instead of introducing a new target there would be simpler/better. What do you think?

I prefer a separate target because then it's a lot easier to run the unittest target with the currently compiled dmd binary. Imho it's a common concept to use a different binary for the unittest executable and deviating from this might lead to confusion or annoyance down the road.

Alright, you make a good point.

andralex

Couple optional suggestions for later.

andralex · 2017-12-07T13:52:24Z

circleci.sh

    make -j$N -C src -f posix.mak MODEL=$MODEL HOST_DMD=../_${build_path}/host_dmd clean
    make -j$N -C src -f posix.mak MODEL=$MODEL HOST_DMD=../_${build_path}/host_dmd ENABLE_COVERAGE=1

+    cp $build_path/dmd _${build_path}/host_dmd_cov


Would a ln suffice here?

Well, the idea was too avoid any issues with rebuilding DMD with other configs. At the moment there aren't any anymore for this setup, but explicitly really helped a lot during debugging here.

andralex · 2017-12-07T13:53:18Z

posix.mak


 all:
-	$(QUIET)$(MAKE) -C src -f posix.mak
+	$(QUIET)$(MAKE) -C src -f posix.mak all


Is there a disadvantage to assume the default target builds all? After all, if it's not it means we wrote that makefile poorly.

marler8997 · 2017-12-12T16:22:04Z

Windows debug build is currently broken because dmd does not generate the 'main' function. I bisected the cause to this commit. I've submitted a PR (#7431) to fix it by adding the main method back in but I'm not sure if that's the right solution since I don't quite understand why it was removed.

wilzbach · 2017-12-12T17:26:48Z

to fix it by adding the main method back in but I'm not sure if that's the right solution since I don't quite understand why it was removed.

I'm copy/pasting my response from #7431:

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:

dmd/src/win32.mak

Line 135 in 7c888c4

DDEBUG=-debug -g -unittest

wilzbach added Merge:Blocked Review:Needs Work labels May 10, 2017

wilzbach mentioned this pull request May 10, 2017

Fix ASTNull #6720

Merged

wilzbach force-pushed the run_unittests branch 2 times, most recently from ab34732 to 0263062 Compare June 11, 2017 04:16

wilzbach force-pushed the run_unittests branch from 0263062 to a5a5d57 Compare November 28, 2017 18:22

wilzbach removed Merge:Blocked Review:Needs Work labels Nov 28, 2017

andralex reviewed Nov 29, 2017

View reviewed changes

wilzbach force-pushed the run_unittests branch from a5a5d57 to 5b58a62 Compare December 1, 2017 04:46

wilzbach changed the title ~~[WIP, do not merge] Run dmd internal unittests on CIs~~ Run dmd internal unittests on CIs Dec 1, 2017

wilzbach force-pushed the run_unittests branch from 5b58a62 to 718d279 Compare December 1, 2017 04:48

wilzbach commented Dec 1, 2017

View reviewed changes

wilzbach force-pushed the run_unittests branch 4 times, most recently from 3ff6b79 to 45e98e6 Compare December 5, 2017 03:49

wilzbach mentioned this pull request Dec 5, 2017

Export HOST_DC during test_dmd braddr/at-client#6

Merged

wilzbach force-pushed the run_unittests branch 3 times, most recently from b0ed6d7 to aa9b521 Compare December 5, 2017 04:12

wilzbach force-pushed the run_unittests branch from aa9b521 to f880ed8 Compare December 5, 2017 05:05

wilzbach mentioned this pull request Dec 5, 2017

Set hard-coded src/dmd executables to generated/ (v2) braddr/at-client#5

Merged

wilzbach force-pushed the run_unittests branch from f880ed8 to 262f44b Compare December 5, 2017 16:55

wilzbach requested a review from CyberShadow as a code owner December 5, 2017 16:55

wilzbach requested review from MartinNowak and WalterBright as code owners December 5, 2017 16:55

wilzbach force-pushed the run_unittests branch from 262f44b to a2025df Compare December 7, 2017 05:03

Run dmd internal unittests on CIs

fed25ec

wilzbach force-pushed the run_unittests branch from a2025df to fed25ec Compare December 7, 2017 05:04

PetarKirov reviewed Dec 7, 2017

View reviewed changes

PetarKirov approved these changes Dec 7, 2017

View reviewed changes

andralex approved these changes Dec 7, 2017

View reviewed changes

andralex merged commit 4af63f9 into dlang:master Dec 7, 2017

wilzbach deleted the run_unittests branch December 12, 2017 17:25

Uh oh!

Conversation

wilzbach commented May 10, 2017

Uh oh!

andralex commented Nov 17, 2017

Uh oh!

dlang-bot commented Nov 28, 2017

Bugzilla references

Uh oh!

wilzbach commented Nov 28, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

braddr commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Dec 5, 2017

Uh oh!

wilzbach commented Dec 6, 2017

Uh oh!

braddr commented Dec 7, 2017

Uh oh!

wilzbach commented Dec 7, 2017

Uh oh!

PetarKirov Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Dec 12, 2017

Uh oh!

wilzbach commented Dec 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PetarKirov Dec 7, 2017 •

edited

Loading