Skip to content

Comments

Move avg and impvisitor examples to test/dub_package#10552

Merged
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:moveExamples
Nov 17, 2019
Merged

Move avg and impvisitor examples to test/dub_package#10552
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:moveExamples

Conversation

@marler8997
Copy link
Contributor

@marler8997 marler8997 commented Nov 10, 2019

With this change the avg and impvisitor examples are now tested by ci.sh instead of from the Makefiles. This is a result of a suggestion from @wilzbach (#10504 (comment))

Note that I modified ci.sh to only build impvisitor.d instead of also running it. This is because it fails to run because of:

/home/runner/dmd/test/dub_package/../../../phobos/std/typetuple.d(42): Error: Outside Unicode code space

However, we currently aren't running impvisitor anyway.

@marler8997 marler8997 force-pushed the moveExamples branch 3 times, most recently from bae9741 to 08f3597 Compare November 10, 2019 05:38
@marler8997 marler8997 changed the title Move avg and impvisitor examples to test/dub_package Move build-example to build.d Nov 10, 2019
@marler8997 marler8997 force-pushed the moveExamples branch 4 times, most recently from 3438fa3 to 29423fc Compare November 10, 2019 06:18
@marler8997 marler8997 changed the title Move build-example to build.d Move avg and impvisitor examples to test/dub_package Nov 10, 2019
@marler8997 marler8997 marked this pull request as ready for review November 10, 2019 06:44
@wilzbach wilzbach closed this Nov 10, 2019
@wilzbach
Copy link
Contributor

wilzbach commented Nov 10, 2019

Sorry, but as the last PR was merged so quickly without a discussion I think I have to be harsher here.
As explained on other PRs, the entire DMD as a library build was a PoC and is not used by a single person as there's dub now. The idea of a new build script was to remove the old clutter not aggregate it.
The examples can be moved into test/dub_examples.

@marler8997
Copy link
Contributor Author

@wilzbach I'm confused, this PR is moving the examples to test/dub_packages like you asked. Why did you close it?

@wilzbach wilzbach reopened this Nov 10, 2019
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @marler8997! 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 coverage diff by visiting the details link of the codecov check)
  • 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#10552"

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I was too fast here.

@MoonlightSentinel
Copy link
Contributor

Maybe remove the corresponding TODOs from build.d:

dmd/src/build.d

Lines 552 to 558 in 65d4a7c

case "check-examples":
"TODO: cxx-unittest".writeln; // TODO
break;
case "build-examples":
"TODO: build-examples".writeln; // TODO
break;

@marler8997 marler8997 force-pushed the moveExamples branch 2 times, most recently from 56c5237 to 247b012 Compare November 11, 2019 04:11
@dlang-bot dlang-bot merged commit 33869c9 into dlang:master Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants