Add `deinitializeDMD` function to deinitialize the front end #8528

jacob-carlborg · 2018-07-31T10:02:08Z

This is a start of being able to restore the global state initialized by initDMD to its original state. It only deinitializes state initialized by initDMD, state store directly inside functions are not restored.

This is useful to invoke the compiler multiple times within the same application. This allows to write more fine grained tests for the compiler, more of a unit test style compared to the current end to end tests.

dlang-bot · 2018-07-31T10:02:09Z

Thanks for your pull request and interest in making D better, @jacob-carlborg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8528"

wilzbach

I think we should add the unittests directly in the source code. That way they are also automatically run on all platforms that the auto-tester supports and are easier to find and we could even shown some of them in the documentation.

ci.sh

src/dmd/builtin.d

test/unit/source/deinitialization.d

wilzbach · 2018-07-31T15:58:00Z

test/unit/source/deinitialization.d

+    immutable init = global.init;
+    assertStructsEqual(global, init);
+
+    global._init();


Partially OT: What do you think about renaming this to initialize, s.t. the pattern matches with the other modules?

And also for the same reason we renamed TypeInfo.init to TypeInfo.initializer.

Yes, I would like to rename all _init to initialize. That's why I named it deinitialize and not deinit. This will get rid of the ugly underscore prefix and spell out the full name instead of an abbreviation. A double win, in my opinion 😃.

jacob-carlborg · 2018-08-01T10:07:10Z

I think we should add the unittests directly in the source code. That way they are also automatically run on all platforms that the auto-tester supports and are easier to find and we could even shown some of them in the documentation

I think there will be too much code in the existing modules. If we would covert all the existing tests to unit tests directly in the existing modules it would add another 200k lines. I think it still will be difficult to test individual functions. I'm thinking we need to start with tests that uses the front end to lex and parse and perhaps run semantic analysis. The tests would then do some assertions on the AST. If there's not a single function that is being it will be more difficult to find a good place to put the test. There will not always be an obvious module to place it in.

jacob-carlborg · 2018-08-04T07:16:56Z

@wilzbach Dub is not installed on the auto-tester machines. Can we installed that or do I need to find a different approach?

wilzbach · 2018-08-04T09:40:37Z

Dub is not installed on the auto-tester machines. Can we installed that or do I need to find a different approach?

That's a question for @braddr.

thewilsonator · 2018-11-10T09:58:02Z

@jacob-carlborg would it be possible to have the tests be a GitHub project and test it with build kite, substituting dmd master with the merge of the PR being tested?

wilzbach · 2018-11-10T13:29:53Z

would it be possible to have the tests be a GitHub project and test it with build kite, substituting dmd master with the merge of the PR being tested?

Would be a pain to upgrade them whenever this still unstable frontend API changes.

wilzbach · 2018-11-10T13:31:36Z

I guess the only way for now is to only run these tests on CircleCi.

jacob-carlborg · 2018-11-10T14:31:33Z

I was planning to replace Dub with a D script. That should work everywhere?

wilzbach · 2018-11-10T22:06:20Z

Yes, but what do you mean by replace? Isn't this partially already done by build.d?

jacob-carlborg · 2018-11-11T09:33:40Z

Yes, but what do you mean by replace? Isn't this partially already done by build.d?

Currently test/run.d and test/Makefile has been modified to run Dub which will execute all unit tests I've added in this PR. Instead of using Dub, I was going to replace that with a plain D script, since Dub isn't available on the auto-tester. I don't think build.d builds and runs the tests?

wilzbach · 2018-11-13T01:46:23Z

Ah good. I thought you were talking about this, but I wasn't sure.

wilzbach

Looks great! Just a few minor nits

a lot of (probably necessary) normalization happens here
consistency unittest vs. unit_test vs. unit_tests (this PR uses all three)

test/README.md

test/run.d

wilzbach · 2019-02-05T14:20:05Z

test/run.d

+        };
+
+        return target;
+    }


This won't play nice with rebuilds.

Any suggestions?

It's not that important as we currently don't have many tests, but in the future we probably have to do:

write a file after every invocation

lastEdit = MAX(timestamp of src/*.d)

compare lastEdit with the timestamp of the written file

(that's how the current testsuite does it, except that it checks the timestamp of the DMD binary which wouldn't be helpful here.)

That could work, but it would complicate the unit test runner even more. It would also need to check unit/*.d.

test/run.d

test/unit_test_runner.d

test/Makefile

test/README.md

jacob-carlborg · 2019-02-05T16:33:50Z

Looks great! Just a few minor nits

A few? 😃

jacob-carlborg · 2019-02-05T16:45:46Z

consistency unittest vs. unit_test vs. unit_tests (this PR uses all three)

I don't use unittest anywhere except for when using the D keyword unittest (or version identifier).

Here's the deal. In English it's called "unit test", it's also what the D spec calls it in the title [1]. Therefore I use "unit test" or "unit tests" (when it's plural) everywhere it's possible. It's not possible to use "unit test" for an identifier in D source code, therefore the convention is to use camel casing, i.e. unitTest. It's possible to use spaces in arguments to applications/scripts but that's not common practice so I use unit_tests instead. The makefile also uses underscores in all the target names. As for the name "unit_test_runner", I think it sounds weird with "unit_tests_runner", but I'm not a native English speaker so what do I know.

[1] https://dlang.org/spec/unittest.html

test/Makefile

test/README.md

test/run.d

test/tools/paths.d

test/run.d

test/tools/paths.d

jacob-carlborg · 2019-02-06T19:06:23Z

Well my point was that this value is a constant and could be defined as such statically.

Fixed (not sure why I can't reply inline to #8528 (comment)).

wilzbach

LGTM! (just a few final questions out of interest).

wilzbach · 2019-02-07T11:15:22Z

test/tools/unit_test_runner.d

+    runConfig();
+    buildStrtold();
+    execute(dmdPath, "@" ~ cmdfilePath);
+    execute(outputPath);


So I presume that you save the commands to a file because you want to be able to invoke DMD outside of this tool, but why not add -run to the command flags above?

So I presume that you save the commands to a file because you want to be able to invoke DMD outside of this tool

The assumption is that there might be a lot of files added and I think Windows has a limitation on how many arguments can be passed to an application.

but why not add -run to the command flags above?

It didn't work. Seems to be some kind of bug. I didn't mange to create a small test case.

wilzbach · 2019-02-07T11:19:34Z

test/tools/unit_test_runner.d

+        cmd ~= "/etc";
+
+    execute(cmd);
+}


Why do we need this? This file will be generated as part of either building DMD or building DMD as a library with Dub.

This is not using Dub anymore. I failed to get it to install. But it might not be required since the assumption is that DMD is built anyway. I'll see if I can remove it.

wilzbach · 2019-02-07T11:25:20Z

test/run.d

+        };
+
+        return target;
+    }


It's not that important as we currently don't have many tests, but in the future we probably have to do:

write a file after every invocation

lastEdit = MAX(timestamp of src/*.d)

compare lastEdit with the timestamp of the written file

(that's how the current testsuite does it, except that it checks the timestamp of the DMD binary which wouldn't be helpful here.)

wilzbach · 2019-02-07T11:28:19Z

BTW as this does have an impact on the testsuite and tooling based on it it would be amazing if @CyberShadow or @MartinNowak could have a quick look over this too.

CyberShadow · 2019-02-07T11:31:07Z

My first impression: that's a lot of kinds of changes in a single commit. I would suggest splitting the changes into separate commits (or PRs, as often argued by Walter), with one kind of change in one commit.

jacob-carlborg · 2019-02-07T12:24:00Z

that's a lot of kinds of changes in a single commit.

There are two kinds of changes: a unit test runner is added and a new function to deinitialize the frontend. What complicates things are that we have five CI systems and two different ways to invoke the tests.

I would suggest splitting the changes into separate commits (or PRs, as often argued by Walter), with one kind of change in one commit.

I bet if I do that there will be complains that code is added that doesn't do anything.

CyberShadow · 2019-02-07T12:28:18Z

I haven't really been following this side of things.

As for the components I'm responsible for, Digger just runs make (with a properly set up environment) in the dmd/test directory. As long as that keeps working, nothing there should break.

There are two kinds of changes: a unit test runner is added

Can this be done in a separate commit or PR?

jacob-carlborg · 2019-02-07T15:38:34Z

Can this be done in a separate commit or PR?

Yes, anything is possible.

jacob-carlborg · 2019-02-07T21:11:30Z

Created a new PR with only the unit test runner: #9333.

jacob-carlborg · 2019-02-08T10:46:33Z

This PR now contains two commits, one which adds the unit test runner and one which adds the deinitialization function. When #9333 is merged I'll rebase this PR and the unit test runner commit will be gone from this PR.

jacob-carlborg · 2019-02-08T10:46:59Z

~~Blocked by: #9333.~~ #9333 has been merged.

This is a start of being able to restore the global state initialized by `initDMD` to its original state. It only deinitializes state initialized by `initDMD`, state store directly inside functions are not restored. This is useful to invoke the compiler multiple times within the same application. This allows to write more fine grained tests for the compiler, more of a unit test style compared to the current end to end tests.

jacob-carlborg · 2019-02-10T20:46:56Z

Is blocking: #9350.

jacob-carlborg requested review from ibuclaw, MartinNowak and WalterBright as code owners July 31, 2018 10:02

wilzbach reviewed Jul 31, 2018

View reviewed changes

jacob-carlborg force-pushed the reset-state branch 2 times, most recently from e939e4b to 886d5dc Compare August 3, 2018 19:23

wilzbach mentioned this pull request Aug 16, 2018

Expose Dub binary to the Make build scripts braddr/d-tester#73

Open

thewilsonator added Merge:Blocked Review:Needs Work Review:Needs Rebase labels Nov 7, 2018

jacob-carlborg force-pushed the reset-state branch from 886d5dc to f55d206 Compare January 15, 2019 19:18

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jan 15, 2019

jacob-carlborg force-pushed the reset-state branch 5 times, most recently from efc0f48 to 1341200 Compare January 16, 2019 20:24

jacob-carlborg mentioned this pull request Jan 20, 2019

Fix Dub package when cross-compiling #9275

Merged

wilzbach reviewed Feb 5, 2019

View reviewed changes

jacob-carlborg force-pushed the reset-state branch 2 times, most recently from 41a7a30 to 10f87db Compare February 6, 2019 11:54

wilzbach reviewed Feb 6, 2019

View reviewed changes

jacob-carlborg force-pushed the reset-state branch from 10f87db to a3fe274 Compare February 6, 2019 19:03

wilzbach approved these changes Feb 7, 2019

View reviewed changes

jacob-carlborg mentioned this pull request Feb 8, 2019

Add unit test runner #9333

Merged

jacob-carlborg force-pushed the reset-state branch from a3fe274 to 16ec85c Compare February 8, 2019 10:44

jacob-carlborg added the Merge:Blocked label Feb 8, 2019

jacob-carlborg force-pushed the reset-state branch from 16ec85c to acc1249 Compare February 9, 2019 15:07

jacob-carlborg force-pushed the reset-state branch from acc1249 to 46c27f1 Compare February 10, 2019 20:21

jacob-carlborg added Review:Blocking Other Work review and pulling should be a priority and removed Merge:Blocked labels Feb 10, 2019

jacob-carlborg mentioned this pull request Feb 10, 2019

Add customizable diagnostic reporter for the lexer and parser #9350

Merged

thewilsonator added the Merge:auto-merge label Feb 11, 2019

dlang-bot merged commit 6e37d62 into dlang:master Feb 11, 2019

jacob-carlborg deleted the reset-state branch February 11, 2019 05:37

Add deinitializeDMD function to deinitialize the front end #8528

Add deinitializeDMD function to deinitialize the front end #8528

Conversation

jacob-carlborg commented Jul 31, 2018

dlang-bot commented Jul 31, 2018

Bugzilla references

Testing this PR locally

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg commented Aug 1, 2018 • edited Loading

jacob-carlborg commented Aug 4, 2018

wilzbach commented Aug 4, 2018

thewilsonator commented Nov 10, 2018

wilzbach commented Nov 10, 2018

wilzbach commented Nov 10, 2018

jacob-carlborg commented Nov 10, 2018

wilzbach commented Nov 10, 2018

jacob-carlborg commented Nov 11, 2018

wilzbach commented Nov 13, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg commented Feb 5, 2019

jacob-carlborg commented Feb 5, 2019

jacob-carlborg commented Feb 6, 2019

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Feb 7, 2019

CyberShadow commented Feb 7, 2019 • edited Loading

jacob-carlborg commented Feb 7, 2019 • edited Loading

CyberShadow commented Feb 7, 2019

jacob-carlborg commented Feb 7, 2019

jacob-carlborg commented Feb 7, 2019

jacob-carlborg commented Feb 8, 2019

jacob-carlborg commented Feb 8, 2019 • edited Loading

jacob-carlborg commented Feb 10, 2019

Add `deinitializeDMD` function to deinitialize the front end #8528

Add `deinitializeDMD` function to deinitialize the front end #8528

jacob-carlborg commented Aug 1, 2018 •

edited

Loading

CyberShadow commented Feb 7, 2019 •

edited

Loading

jacob-carlborg commented Feb 7, 2019 •

edited

Loading

jacob-carlborg commented Feb 8, 2019 •

edited

Loading