Move man to build.d by marler8997 · Pull Request #10581 · dlang/dmd

marler8997 · 2019-11-17T19:39:15Z

No description provided.

dlang-bot · 2019-11-17T19:39:17Z

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#10581"

MoonlightSentinel

Some suggestions to improve readability and plattform independency

src/build.d

MoonlightSentinel · 2019-11-18T13:02:28Z

src/build.d

+            .deps([mkdirDep(manFileDep.target.dirName)])
+            .command(["cp", manFileDep.sources[0], manFileDep.target])
+            .msg(manFileDep.command.join(" "))
+        ))


Probably better as a local function/lambda before man. That would clearly seperate these dependencies and will probably be easier when using buildPath

(Comment is referring to the entire map-lambda. Apparently GitHub does not handle multiline review comments - or i messed up)

MoonlightSentinel · 2019-11-18T13:25:55Z

src/build.d

+            .target(genManDir.buildPath(e))
+            .sources([dmdRepo.buildPath("docs/man", e)])
+            .deps([mkdirDep(manFileDep.target.dirName)])
+            .command(["cp", manFileDep.sources[0], manFileDep.target])


Suggested change

.command(["cp", manFileDep.sources[0], manFileDep.target])

.commandFunction(() { copy(manFileDep.sources[0], manFileDep.target); })

I've done this, but now when the copy occurs the timestamps aren't updated. So on my machine, these dependencies will get executed every time I build the "man" target...

I've created the copyAndTouch function which makes sure to update the target timestamp using touch on linux platforms.

src/build.d

wilzbach · 2019-11-21T08:34:46Z

src/posix.mak

 ################################################################################

-DMD_MAN_PAGE = $(GENERATED)/docs/man1/dmd.1
+DMD_MAN_PAGE = $(GENERATED)/docs/man/man1/dmd.1


This variable is now unused.

Actually it's still used in the "install" target a few lines down.

wilzbach · 2019-11-21T08:36:12Z

BTW we also upload the man page to DAutoTest, e.g. http://dtest.dlang.io/artifact/website-08a5fe1c6137cc5879028d65e8b1ee42781468bc-e3a514cecad8d5bfab69f12f20f39d08/web/docs/man/man1/dmd.1

And DAutoTest reporting zero changes is already a good sign 👍

MoonlightSentinel · 2019-11-21T10:19:31Z

Maybe man (and other targets relying on D scripts) should be imported instead of compiling and executing a separate executable.
The obvious drawback would be a slight increase in build.ds compile time because of the additional files but it would also provide several benefits:

Less code to maintain (as the compilation target can be omitted)
Reduced execution time (no execute overhead)
Better parallelization while respecting the maximum amount of jobs (once its implemented)

src/build.d

MartinNowak · 2019-12-04T20:22:03Z

Looks like this broke all dlang.org builds with @CyberShadow's website build tool, e.g. http://dtest.dlang.io/results/38f11f93084f7f0c502f190deba166837646510a/61fe0b3552e74ce37f4e75b0d5851335c963cbbe/.
Seems to be triggered by gen_man relying phobos, while that is not available in the build tool during the dmd build. Can we do sth. about this to unblock publishing releases/updating the website @marler8997, @wilzbach, @RazvanN7?

MoonlightSentinel · 2019-12-04T20:42:17Z

@MartinNowak This issue arises because the Makefile invoking build.d does not pass the DMD variable and and uses dmd from path. See #10637

marler8997 requested review from MartinNowak and wilzbach as code owners November 17, 2019 19:39

marler8997 force-pushed the moveMan branch 8 times, most recently from aebcefe to 99faa4d Compare November 17, 2019 20:22

MoonlightSentinel reviewed Nov 18, 2019

View reviewed changes

marler8997 force-pushed the moveMan branch from 99faa4d to 2a5ca21 Compare November 18, 2019 19:47

wilzbach reviewed Nov 21, 2019

View reviewed changes

src/build.d Outdated Show resolved Hide resolved

wilzbach reviewed Nov 21, 2019

View reviewed changes

marler8997 force-pushed the moveMan branch 3 times, most recently from 7562784 to 011393a Compare November 21, 2019 10:10

marler8997 force-pushed the moveMan branch 2 times, most recently from 5719369 to dc0c245 Compare November 21, 2019 11:02

MoonlightSentinel reviewed Nov 21, 2019

View reviewed changes

src/build.d Show resolved Hide resolved

MoonlightSentinel reviewed Nov 21, 2019

View reviewed changes

src/build.d Outdated Show resolved Hide resolved

MoonlightSentinel reviewed Nov 21, 2019

View reviewed changes

src/build.d Outdated Show resolved Hide resolved

marler8997 force-pushed the moveMan branch from dc0c245 to 4b379b9 Compare November 21, 2019 18:00

Move man to build.d

4646428

marler8997 force-pushed the moveMan branch from 4b379b9 to 4646428 Compare November 21, 2019 18:04

RazvanN7 approved these changes Nov 28, 2019

View reviewed changes

thewilsonator approved these changes Nov 28, 2019

View reviewed changes

thewilsonator added the Merge:auto-merge label Nov 28, 2019

dlang-bot merged commit 4431f19 into dlang:master Nov 28, 2019

	.command(["cp", manFileDep.sources[0], manFileDep.target])
	.commandFunction(() { copy(manFileDep.sources[0], manFileDep.target); })

Uh oh!

Comments

Conversation

marler8997 commented Nov 17, 2019

Uh oh!

dlang-bot commented Nov 17, 2019

Bugzilla references

Testing this PR locally

Uh oh!

MoonlightSentinel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoonlightSentinel Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

marler8997 Nov 21, 2019 • 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!

marler8997 Nov 21, 2019 • 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!

Uh oh!

wilzbach Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

marler8997 Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Nov 21, 2019

Uh oh!

MoonlightSentinel commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinNowak commented Dec 4, 2019

Uh oh!

MoonlightSentinel commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

marler8997 Nov 21, 2019 •

edited

Loading

marler8997 Nov 21, 2019 •

edited

Loading

MoonlightSentinel commented Nov 21, 2019 •

edited

Loading

MoonlightSentinel commented Dec 4, 2019 •

edited

Loading