Fix issue 17392 - Add Dub file for the lexer and parser by jacob-carlborg · Pull Request #6771 · dlang/dmd

jacob-carlborg · 2017-05-12T08:48:32Z

The Dub file is intended to make the the compiler available as a library through Dub.

Currently the Dub package is only tested in Travis CI. I don't know if Dub is available in the autotester or not.

dlang-bot · 2017-05-12T08:48:34Z

Thanks for your pull request, @jacob-carlborg! 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

Auto-close	Bugzilla	Description
✓	17392	Add Dub file for the lexer and parser

jacob-carlborg · 2017-05-12T16:34:05Z

@andralex @RazvanN7 @MartinNowak ping.

jacob-carlborg · 2017-05-12T16:34:37Z

Someone from the D foundation or the D core team would need to create an account on code.dlang.org to make the Dub package available.

CyberShadow · 2017-05-13T13:49:39Z

dub.sdl

+sourcePaths "docs"
+
+// This is required because Dub needs to have at lease one source file
+sourceFiles "src/ddmd/package.d"


Is there really no better solution here? Having to put hacks for our official package manager in our official compiler's package description is ... not great.

As far as I can see, changing to targetType "none" would allow dropping the sourcePaths and sourceFiles directives and should otherwise behave the same (except that it doesn't generate the extra dummy library).

Then it's not possible to build the main package:

Main package must have a binary target type, not none. Cannot build.

Although it seems like it can still be used as a dependency without any problems. But the natural dub build doesn't work anymore.

Fixed, but not sure if it's correct.

True, that's indeed a difference. But dub build would only build the dummy library. Dependencies are only built for dynamic library or executable targets. There is dlang/dub#97, which would solve this issue.

Aha, I see. At some point an executable or dynamic library needs to be built for a library to be useful. So I guess it kind of solves itself.

CyberShadow · 2017-05-13T13:51:15Z

CC @s-ludwig @wilzbach

CyberShadow · 2017-05-13T20:32:04Z

dub.sdl

+subPackage {
+  name "parser"
+  targetType "library"
+  sourcePaths "docs"


@s-ludwig Thanks! Is there a similar solution here to avoid the dummy sourcePaths, where only two sourceFiles are needed?

sourcePaths "docs" can be replaced by an empty sourcePaths. Defining an empty list of source paths is enough to inhibit the auto-detection of the "src" folder.

That worked, thanks.

jacob-carlborg · 2017-05-15T19:39:41Z

Anything else that needs to be taken care of before this can be merged?

andralex · 2017-05-15T20:46:36Z

Cool beans! I'm on board with this, though it's a bit early :o). @MartinNowak could you do the honors of merging if all looks good?

jacob-carlborg · 2017-05-16T07:22:36Z

Cool beans! I'm on board with this, though it's a bit early :o)

Yes, but if we are very clear that we don't provide any stable API between release and use version 0.x.y, I don't think it's any problem. Semantic versioning says:

"Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable"

We can append the DMD version as metadata, i.e. 0.0.1+2.075.0. Hopefully this can speed up the process of making the frontend available as a library, since it will be more easily accessible. Two pull requests have already broken the separation between the parser and the rest of the frontend. The tests in this PR would catch that.

yebblies · 2017-05-21T06:28:39Z

The problems with idgen really need to be fixed instead of hacked around. It looks like we can replace idgen with ctfe, since we don't need id.h any more. Any (LDC/GDC) glue uses could use idPool anyway.

ibuclaw · 2017-05-21T07:51:33Z

This uses sdl :sadder:

jacob-carlborg · 2017-05-21T18:46:35Z

The problems with idgen really need to be fixed instead of hacked around. It looks like we can replace idgen with ctfe, since we don't need id.h any more. Any (LDC/GDC) glue uses could use idPool anyway.

Can we wait with that for another pull request?

yebblies · 2017-05-22T02:52:14Z

Can we wait with that for another pull request?

Why? This PR should wait for that PR IMO, since nobody is likely to actually use this until we have at least a working parser exposed. (See the existing dub package that exposes the lexer.) I don't want the idgen hacks in master.

jacob-carlborg · 2017-05-22T08:03:53Z

Why?

Because I'm trying to keep the PR as small as possible. I've learned the hard way to not create too big PR's or change too much.

yebblies · 2017-05-22T08:12:10Z

Yeah I'm not asking for that fix in this PR, I want it done in another PR first before this PR is merged. Which would end up making this PR smaller.

jacob-carlborg · 2017-05-28T18:59:57Z

Yeah I'm not asking for that fix in this PR, I want it done in another PR first before this PR is merged. Which would end up making this PR smaller.

@yebblies I created #6837 for this.

UplinkCoder · 2017-05-28T22:16:28Z

@jacob-carlborg idgen can be run as a prebuild step.
Or
we ship a pre-generated version of id.d
Which can be updated by idgen whenever idgen changes.

jacob-carlborg · 2017-05-29T06:44:09Z

idgen can be run as a prebuild step.

@UplinkCoder it already is run as a prebuild step [1], but #6771 (comment). I don't care which approach we use, please just come to an agreement.

[1] https://github.com/dlang/dmd/pull/6771/files#diff-406ece895a5b70a272cd76a2ef902836R41

jacob-carlborg · 2017-06-03T19:01:50Z

@yebblies So Walter didn't like that the a header file wasn't generated anymore #6837. Would you like to try convince him or is the solution in this PR acceptable?

wilzbach · 2017-06-05T16:21:08Z

@jacob-carlborg FYI: dlang/dlang.org#1671 will expose the documentation pages for dmd on dlang.org ;-)

The Dub file is intended to make the the compiler available as a library through Dub.

wilzbach · 2017-07-16T17:37:25Z

Btw the failure of gdc on Travis wasn't spurious

See also #6999 for using the built compiler to run the test instead of the host compiler as all other test in this file do.

wilzbach · 2017-07-17T02:09:14Z

Btw @MartinNowak @CyberShadow (see private email for credentials) - I tried to add dlang/dmd to code.dlang.org, but I got this nice error:

So how about pushing v0.0.1 as long as SemVer is still WIP?

ibuclaw · 2017-07-17T06:34:19Z

Btw the failure of gdc on Travis wasn't spurious.

Well you are using an older version of gdc. The failure itself is dmd frontend that doesn't have the same c++ support as more recent versions.

jacob-carlborg · 2017-07-17T06:38:05Z

So how about pushing v0.0.1 as long as SemVer is still WIP?

@wilzbach We could also set the version scheme that is currently used as metadata, i.e. v0.0.1+2.075.0.

ibuclaw · 2017-07-17T06:46:03Z

By the way, doesn't the auto-merge label prevent broken PRs from being committed? FYI @andralex on why you should not use merge directly.

CyberShadow · 2017-07-17T06:51:36Z

@ibuclaw See dlang/dlang-bot#69

ibuclaw · 2017-07-17T06:56:55Z

Hmm, and I suppose the autotester only merges if all of its tests pass. So I guess I'm misremembering a time when this was not the case. ☺️

ibuclaw · 2017-07-17T07:00:26Z

In the gdc repo, I protect master, and make it so that not even admins can force a merge if any required tests are failing.

Couldn't this be applied here too?

CyberShadow · 2017-07-17T07:01:35Z

Yes, we do that too. Jenkins is not currently required because Dub/Vibe tests fail sporadically.

jacob-carlborg · 2017-07-17T07:06:01Z

@CyberShadow but in this case the Jenkins tests passed but not Travis (gdc)

CyberShadow · 2017-07-17T07:07:16Z

Oops, that's right.

Travis is not set as required here either. I don't know why, though.

jacob-carlborg · 2017-07-17T07:11:52Z

IIRC, someone said that the macOS test on Travis were failing from time to time.

jacob-carlborg · 2017-07-17T07:15:20Z

Not sure if it's possible to specify some of the tests from Travis to be required.

ibuclaw · 2017-07-17T07:50:36Z

Travis has allow_failures or something akin to.

wilzbach · 2017-07-17T09:36:30Z

IIRC, someone said that the macOS test on Travis were failing

It's not only that, we also have random failures due to

dlang.org being unreachable (it got a lot better since we use a second server as fallback, but it's still an issue sometimes
code.dlang.org being unreachable (the dub binary is fetched from there even though it's nowadays bundled with DMD and LDC)
random failures in the phobos testsuite
reaching maximum run time
failing to do any output on OSX
...

Just browse https://travis-ci.org/dlang/dmd/builds yourself.
So I think those were the reasons against setting it to required.

Travis has allow_failures or something akin to.

Yes, but allowing failures for all builds isn't very useful.

jacob-carlborg · 2017-07-17T11:09:44Z

Just browse https://travis-ci.org/dlang/dmd/builds yourself.

Basically all of the ones I looked at was failing due to exceeding the time limit. One was failing due to code.dlang.org not being available.

ibuclaw · 2017-07-17T11:19:49Z

I thought builds averaged 20 minutes. Is travisci that terrible now? Or is the CI tests just doing that much more.

wilzbach · 2017-07-17T19:35:50Z

So how about pushing v0.0.1 as long as SemVer is still WIP?

Martin was a bit worried that this might jeopardize the plans to rectify SemVer at DMD and confuse users.

@s-ludwig would it be possible to add DMD manually, s.t. only its branches can be used? I guess ~master should be enough in the beginning to start testing this?

s-ludwig · 2017-07-17T19:41:26Z

Easiest would be to add a dummy v0.0.1 tag, add it, and then remove the tag again. It will stay registered in that case.

wilzbach · 2017-07-17T19:45:10Z

Easiest would be to add a dummy v0.0.1 tag, add it, and then remove the tag again. It will stay registered in that case.

OK temporarily added v0.0.1:

https://github.com/dlang/dmd/releases/tag/v0.0.1

But I am still getting the error message - do I need to wait until it the cache is emptied?

s-ludwig · 2017-07-17T19:55:42Z

Hm, just looked at the source and the registry pick up only the last 100 tags (in lexicographical order). Will open a ticket. You can try with v3.0.0 for the time being.

wilzbach · 2017-07-17T20:01:02Z

You can try with v3.0.0 for the time being.

Thanks - seems like this worked. Should be public soon:

wilzbach · 2017-07-17T20:03:20Z

Basically all of the ones I looked at was failing due to exceeding the time limit. One was failing due to code.dlang.org not being available.

That's good. At least our previous work has shown some fruits :)
The DUB failures should be gone once dlang/dub#1190 and dlang/dub#1198 have been merged and deployed.
The timeout issues don't affect PRs that much as fewer arguments are used. So we actually could try to require Travis.

What do you think @CyberShadow?

I thought builds averaged 20 minutes. Is travisci that terrible now? Or is the CI tests just doing that much more.

I think it's a combination: performance of Travis is degrading over time due to more users and constantly new tests are added to the testsuite.

wilzbach · 2017-07-17T20:20:01Z

Should be public soon:

-> http://code.dlang.org/packages/dmd

@jacob-carlborg's unittest modified to an example:

#!/usr/bin/env dub
/++ dub.sdl:
name "dmd_lexer_example"
dependency "dmd" version="~master"
+/
void main()
{
    import ddmd.lexer;
    import ddmd.tokens;
    import std.stdio;

    immutable sourceCode = "void test() {} // foobar";
    scope lexer = new Lexer("test", sourceCode.ptr, 0, sourceCode.length, 0, 0);
 
    while (lexer.nextToken != TOKeof)
        writeln(lexer.token.value);
}

@jacob-carlborg how about making an announcement of your great work?
@RazvanN7 how about using a DUB single file header for your examples?

CyberShadow · 2017-07-17T22:00:52Z

What do you think @CyberShadow?

We can try it I guess. Stable too?

jacob-carlborg · 2017-07-18T09:35:11Z

how about making an announcement of your great work?

Yes. That's a good idea 👍.

dlang-bot added the Severity:Bug Fix label May 12, 2017

jacob-carlborg force-pushed the issue-17392-dub branch from 98a248c to 46c49bd Compare May 12, 2017 16:30

jacob-carlborg changed the title ~~[WIP] Fix issue 17392 - Add Dub file for the lexer and parser~~ Fix issue 17392 - Add Dub file for the lexer and parser May 12, 2017

CyberShadow reviewed May 13, 2017

View reviewed changes

jacob-carlborg force-pushed the issue-17392-dub branch from 46c49bd to 7e832a2 Compare May 14, 2017 13:18

This was referenced May 23, 2017

Remove ASTNull #6759

Merged

Replace idgen with CTFE implementation #6837

Merged

wilzbach mentioned this pull request May 28, 2017

deduplicate the 'isSomeFunction' equivalent #6835

Closed

jacob-carlborg mentioned this pull request Jul 16, 2017

stringImportPaths very slow dlang/dub#1199

Open

Fix issue 17392 - Add Dub file for the lexer and parser

b5e6ed4

The Dub file is intended to make the the compiler available as a library through Dub.

jacob-carlborg force-pushed the issue-17392-dub branch from 7e832a2 to b5e6ed4 Compare July 16, 2017 14:19

wilzbach mentioned this pull request Jul 16, 2017

Convert DUB package test into a Makefile test and run it with the proper compiler flags #6999

Closed

jacob-carlborg deleted the issue-17392-dub branch July 17, 2017 06:35

wilzbach mentioned this pull request Jul 17, 2017

Reassign copyright from Digital Mars to D Language Foundation #7004

Merged

wilzbach mentioned this pull request Dec 9, 2017

Start testing the DMD DUB package on Travis #7415

Merged

Uh oh!

Conversation

jacob-carlborg commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

jacob-carlborg commented May 12, 2017

Uh oh!

jacob-carlborg commented May 12, 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!

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!

CyberShadow commented May 13, 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!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented May 15, 2017

Uh oh!

andralex commented May 15, 2017

Uh oh!

jacob-carlborg commented May 16, 2017

Uh oh!

yebblies commented May 21, 2017

Uh oh!

ibuclaw commented May 21, 2017

Uh oh!

jacob-carlborg commented May 21, 2017

Uh oh!

yebblies commented May 22, 2017

Uh oh!

jacob-carlborg commented May 22, 2017

Uh oh!

yebblies commented May 22, 2017

Uh oh!

jacob-carlborg commented May 28, 2017

Uh oh!

UplinkCoder commented May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob-carlborg commented May 29, 2017

Uh oh!

jacob-carlborg commented Jun 3, 2017

Uh oh!

wilzbach commented Jun 5, 2017

Uh oh!

wilzbach commented Jul 16, 2017

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

ibuclaw commented Jul 17, 2017

Uh oh!

jacob-carlborg commented Jul 17, 2017

Uh oh!

ibuclaw commented Jul 17, 2017

Uh oh!

CyberShadow commented Jul 17, 2017

Uh oh!

ibuclaw commented Jul 17, 2017

Uh oh!

jacob-carlborg commented May 12, 2017 •

edited

Loading

dlang-bot commented May 12, 2017 •

edited

Loading

UplinkCoder commented May 28, 2017 •

edited

Loading