Add assert -> writeln transformation magic for runnable unittest examples by wilzbach · Pull Request #1582 · dlang/dlang.org

wilzbach · 2017-02-22T17:48:32Z

Okay this is a more "sophisticated" approach towards rewriting the AssertExpressions in the runnable examples than the initial regex.

I think we all now the problem: "All test passed" is a quite boring output and not so much than being able to directly change a single character.
The objective of this tool is to be conservative as a broken example looks a lot worse than a few statements that could have potentially been rewritten.
For now:

only EqualExpressions are "transformed"
static asserts are ignored
only single-line asserts are rewritten
this is only enabled for the prerelease pages

However

that's still >12K automatic writeln transformations
as ddoc is used to generate the documentation, it's ensured that no invalid transformations happened.

@CyberShadow I am not sure on the best way to integrate this into the existing Makefile is. At the moment the tool replaces occurrences directly in the Phobos directory which might be a bit inconvenient if enabled as default. Should we make a copy of the Phobos sources before?

This depends on these two fixes to libdparse (hence blocked):

CyberShadow · 2017-02-22T23:38:55Z

This could use a bit more background (for myself included, sorry :)). What problem does this solve again?

From my limited understanding of this it really seems like we are fixing this in the wrong place.

For example, could this go into DPaste instead? Perhaps a CGI proxy between dlang.org and DPaste? I guess fixing this in the compiler/runtime is off the table.

Should we make a copy of the Phobos sources before?

I don't remember offhand if Digger can cope with one repository's build script modifying another repository, but the test service shouldn't complain. Still, it would be confusing to users: what if you're testing a change, build the website, and suddenly your working directory is dirtied with hundreds of changes that you'd have to carefully clean up so you can commit just your intended changes?

wilzbach · 2017-02-23T01:07:53Z

My fixes to libdparse have been merged (thanks a lot for being so reponsive @Hackerpilot), so now you can preview the effects of the script.

std.algorithm.iteration with assert examples:
std.algorithm.iteration with writeln examples

This could use a bit more background (for myself included, sorry :)). What problem does this solve again?

No it's my fault for not explaining it properly!

The idea is that assert's while great for the compiler to test, aren't well-suited for examples as a reader often wants to copy/paste the example and/or play with it. Especially with the new runnable examples this becomes interesting because by default when he clicks on "Run" only "All tests passed" will be shown, which is suboptimal.

So this script tries to rewrite EqualExpressionsof AssertExpressions in Unittest blocks into writeln calls. For example this snippet

assert([2, 1, 4, 3].minElement == 1);
assert([5, 3, 7, 9].enumerate.minElement!"a.value" == tuple(1, 3));

would be rewritten as:

writeln([2, 1, 4, 3].minElement); // 1
writeln([5, 3, 7, 9].enumerate.minElement!"a.value"); // tuple(1, 3))

Providing examples with output is a common style across almost all language and library documentations. A couple of examples:

C
C++ (this one also has a "run" feature)
Ruby
Python
NumPy
GoLang
...

Even all examples on dlang.org (and the examples included in the Ddoc comment) use writeln

CC @MartinNowak and @andralex as they also had the same idea.

For example, could this go into DPaste instead? Perhaps a CGI proxy between dlang.org and DPaste? I guess fixing this in the compiler/runtime is off the table.

Well it's really intended to be shown on dlang.org.

I don't remember offhand if Digger can cope with one repository's build script modifying another repository, but the test service shouldn't complain. Still, it would be confusing to users:

Yeah I am quite aware of this downside, but doing this transformation in JavaScript with the examples rendered as syntax-highlighted HTML didn't turn it to be a great idea.

CyberShadow · 2017-02-23T01:20:20Z

I see, thanks for the explanation.

What do you think about integrating it into DDox?

CyberShadow · 2017-02-23T01:22:17Z

Something seems to have gone terribly wrong here:
http://dtest.dlang.io/artifact/website-e2ad3d92df8634075b168d136f491ff290d735d7-fc1384e8b0cb8ba3361595308b2051e3/web/library-prerelease/std/algorithm/setops/writeln.html

wilzbach · 2017-02-23T01:35:06Z

What do you think about integrating it into DDox?

It seems that the switch to DDox won't happen in the foreseeable near future, so this wouldn't allow us to use this "feature" for the default documentation and thus for the majority of all readers/users.
In any case, DDox already uses a libdparse, so it wouldn't be difficult to integrate this into Ddox if we decide to go this road.

Something seems to have gone terribly wrong here:
http://dtest.dlang.io/artifact/website-e2ad3d92df8634075b168d136f491ff290d735d7-fc1384e8b0cb8ba3361595308b2051e3/web/library-prerelease/std/algorithm/setops/writeln.html

The script automatically adds a writeln stub to every module, so that Ddoc compiles successfully.
I am pretty sure it's due to this, but this symbol is set to private.

CC @s-ludwig

wilzbach · 2017-02-23T01:58:56Z

Something seems to have gone terribly wrong here:

I found & fixed the problem: [setops ends with a /++ comment](https://github.com/dlang/phobos/blob/master/std/algorithm/setops.d#L1272), hence the writeln` symbol is assumed to be public, even though it's set to private.

CyberShadow · 2017-02-23T01:58:59Z

Alright, well, I think this is fine provided that we can avoid modifying Phobos source code in-place with the caveat I described. It's just not great that this puts in an additional step between the source code and the final output, which makes the entire thing more complicated, but if that can't be avoided then so be it. So, if it's not too difficult, would be good if the changes were done to another private copy, either by copying the source files and modifying them in-place, or having the tool read from one location and write to another. Otherwise we should probably exclude this from the default target (maybe we should introduce a new target that specifies that the files are built by DAutoTest, so we'd only have to change DAutoTest once).

CyberShadow · 2017-02-23T02:00:39Z

It's just not great that this puts in an additional step between the source code and the final output,

A PR which does that as well: #1214

Though, I think the justification for that one is much weaker (which is also why it's been stalled forever).

wilzbach · 2017-02-23T02:47:06Z

So, if it's not too difficult, would be good if the changes were done to another private copy,

Sure! I used

rsync --update s.t. only the exact changes will be propagated to ddoc.
wildcard, s.t. changes to source files will be propagated

Also as rsync can't be assumed to be existent, I added a fallback with cp that will copy everything.

CyberShadow · 2017-02-23T07:53:15Z

Yeah, I guess this works, though probably the way I'd do it is a makefile wildcard rule to copy the files that need to be copied. This is probably fine too.

wilzbach · 2017-02-24T03:38:26Z

Yeah, I guess this works, though probably the way I'd do it is a makefile wildcard rule to copy the files that need to be copied. This is probably fine too.

You know, I don't really care. If you want to, I am happy to change ;-)

When I introduced the runnable examples for the prerelease pages, they sat there for half a year without any feedback/notices. Hence I also added this writeln transformation process to the stable pages, because if we decide to go with it, this "testing phase" would be useless. The preview of the changes on DAutoTest is awesome and should be enough to make an informed decision.

CyberShadow · 2017-02-24T03:39:27Z

You know, I don't really care. If you want to, I am happy to change ;-)

No, that's completely fine.

CyberShadow

LGTM aside the rsync22 thing.

CyberShadow · 2017-02-24T03:43:17Z

.gitignore

 /.generated
+
+# DUB binaries
+assert_writeln_magic


Nit: absolute paths (starting with /) are a bit better perfomant

CyberShadow · 2017-02-24T03:44:15Z

assert_writeln_magic.d

+    /// A single line
+    override void visit(const DeclarationOrStatement expr)
+    {
+    processLastAssert();


Weird whitespace

CyberShadow · 2017-02-24T03:45:38Z

posix.mak

+
+# Automatically generated directories
 GENERATED=.generated
+PHOBOS_DIR_GENERATED=../phobos.ddoc.tmp


I'd say that the .generated directory would be a better fit, to avoid more littering

CyberShadow · 2017-02-24T06:46:14Z

posix.mak

+################################################################################
+
+# --update allows to copy only the newer files and thus only propagate these
+#  changes


Why rsync22? Typo? The commands below run rsync.

Is command like which here?

CyberShadow · 2017-02-24T06:48:55Z

assert_writeln_magic.d

+        lines = File(destFile).byLineCopy.array;
+
+        // it's a good practise to use a common tmp folder -> easier to look at or clean
+        tmpDir = buildPath(tempDir, "file_tester", path.stripExtension.replace("/", "_"));


.generated ?

tmpDir is only used when writing the file line by line and doing the swap. It will be immediately be removed afterwards. Isn't this the dedicated purpose of a tempDir() or is there another reason to use .generated?

OK. Currently on DAutoTest .generated is on a RAM disk but /tmp is not, but I should set the TMP variable to fix that.

(I wouldn't mind changing this)

CyberShadow · 2017-02-24T06:49:04Z

assert_writeln_magic.d

+
+    void resetTestState()
+    {
+


wilzbach · 2017-02-24T07:22:34Z

I'd say that the .generated directory would be a better fit, to avoid more littering

I didn't do this initially because of build errors due to the hard-coded paths, but I forgot that one can simply overwrite variables in make :)

Why rsync22? Typo? The commands below run rsync.

Yeah :/
Somehow I was trying whether HAS_RSYNC works and somehow this ended up in the git diff. Sorry

Is command like which here?

Yes
-> http://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script

CyberShadow · 2017-02-24T07:23:52Z

-> http://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script

TIL, thanks.

CyberShadow · 2017-02-24T12:49:54Z

I have no further feedback so the only thing remaining is to squash any comments you think ought to be squashed before merging.

wilzbach · 2017-02-24T14:28:57Z

the only thing remaining is to squash any comments you think ought to be squashed before merging.

Currently ~master of libdparse is fetched as there hasn't been a new release with the bug fixes included (though they have already been merged :)).
It's quite unlikely that @Hackerpilot will break libdparse, but it's still a bad practice to run a CI without a fixed version, so I think we should either wait a bit for a new version (or work around this limitation in DUB by e.g. embedding libdparse as a git submodule.)

CyberShadow · 2017-02-24T14:30:03Z

Does dub allow specifying dependencies by commit hash yet? I think Sönke said he'd do it at some point.

wilzbach · 2017-02-24T14:34:54Z

Does dub allow specifying dependencies by commit hash yet?

Yes it's on the roadmap, but not implemented (as Sönke is quite busy with Vibe.d and other projects).
There's a DEP for it though: https://github.com/dlang/dub/wiki/DEP6

Hackerpilot · 2017-02-24T20:30:11Z

@wilzbach https://github.com/Hackerpilot/libdparse/releases/tag/v0.7.0-beta.7

wilzbach · 2017-02-24T21:05:23Z

@wilzbach https://github.com/Hackerpilot/libdparse/releases/tag/v0.7.0-beta.7

Wow. Thanks a lot!

In order to have a common style, these variables have been introduced DMD_STABLE_DIR=${DMD_DIR}-${LATEST} DRUNTIME_STABLE_DIR=${DRUNTIME_DIR}-${LATEST} PHOBOS_STABLE_DIR=${PHOBOS_DIR}-${LATEST} PHOBOS_STABLE_DIR_GENERATED=${PHOBOS_STABLE_DIR}.ddoc.tmp

wilzbach · 2017-02-26T08:19:59Z

CyberShadow/DAutoTest — Build failed

Arrgh it seems that there was an error with the DUB version. Fixed & it's green again :)

CyberShadow · 2017-02-27T14:56:42Z

Nothing else is blocking this, right?

I'll be looking forward to removing the intermediate source file generation whenever DDox becomes the main documentation method.

BTW, I went ahead and hacked DPaste to accept dtest.dlang.io for cross-side requests, so that previews of runnable examples work.

wilzbach · 2017-02-27T15:07:28Z

Nothing else is blocking this, right?

Not that I would know of.

I'll be looking forward to removing the intermediate source file generation whenever DDox becomes the main documentation method.

Ditto!
It's really a hassle to fight against two documentation engines :/

BTW, I went ahead and hacked DPaste to accept dtest.dlang.io for cross-side requests, so that previews of runnable examples work.

Sweet! Thanks a lot! I haven't been able to reach Damian lately, but my wishlist would include:

support dmd-nightly (for the pre-release pages, e.g. checkedint doesn't work currently)
add a (simple) cache (most users press on "run" without editing)
check whether there's a speed difference in using dmd -run instead of rdmd

CyberShadow · 2017-02-27T15:08:22Z

Sweet! Thanks a lot! I haven't been able to reach Damian lately, but my wishlist would include:

If you want to have a go at it I'll gladly give you access :)

CyberShadow · 2017-02-27T15:12:10Z

Uh oh, the dlang.org autodeploy build failed:

Linking...
Running ./assert_writeln_magic -i .generated/phobos-prerelease
std.file.FileException@std/file.d(2229): /tmp/file_tester/unittest: Permission denied

CyberShadow · 2017-02-27T15:13:58Z

Right, because the same directory is being shared by multiple users.

I'm going to work around this by setting TMP to a new directory under the working directory, which should also improve performance and encapsulation, but going forward I think we should avoid using globally unique file names especially when we need to write to them.

wilzbach · 2017-02-27T15:14:49Z

Uh oh, the dlang.org autodeploy build failed:
...
but going forward I think we should avoid using globally unique file names especially when we need to write to them.

I am happy to make a follow-up PR to change from /tmp to .generated - just let me know.

CyberShadow · 2017-02-27T15:28:29Z

assert_writeln_magic.d

+/**
+A simple line-based in-memory representation of a file.
+ - will automatically write all changes when the objct is destructed
+ - will use a temporary file to do safe, whole file swaps


BTW if you wanted safe, atomic file replacements I think writing to <filename>.tmp and renaming over it would be closer to that goal, since POSIX renames are atomic.

My motivation to save <filename>.tmp in a tmp folder was that the process could be stopped/interrupted during the write of the temporary file.

Yes, but the same applies to your copy call.

Yah, renames are atomic only within the same filesystem which is why it's important to put the xyz.tmp file in the same place as xyz (/tmp might be on a different filesystem etc).

BTW the pattern @CyberShadow has saved my behind many times, and is considered par for the course across Facebook eng.

CyberShadow · 2017-02-27T15:31:23Z

CyberShadow/ae@c919ef2

Seems to be fixed:

https://dlang.org/library-prerelease/std/algorithm/comparison/among.html

MartinNowak · 2017-03-01T14:29:12Z

This broke the builds script b/c your invocation is relying on some system-wide dmd. Please change it to use --compiler=$(STABLE_DMD).

wilzbach · 2017-03-01T14:35:55Z

This broke the builds script b/c your invocation is relying on some system-wide dmd. Please change it to use --compiler=$(STABLE_DMD).

-> #1592

wilzbach added the Blocked label Feb 22, 2017

wilzbach removed the Blocked label Feb 23, 2017

CyberShadow reviewed Feb 24, 2017

View reviewed changes

wilzbach force-pushed the assert_writeln_magic branch from c2f926c to 142acd0 Compare February 24, 2017 14:22

wilzbach force-pushed the assert_writeln_magic branch 2 times, most recently from 88fbd38 to 9d02049 Compare February 24, 2017 21:02

wilzbach added 4 commits February 26, 2017 09:13

Add a conservative assert to writeln transformer

a7232de

Integrate assert_writeln_magic into Phobos-release build

3387076

Bump DUB to 1.1.0 for single file support

f1c041d

set libdparse version to v0.7.0-beta.7

9d1dadf

wilzbach added 6 commits February 26, 2017 09:13

set _writeln stub to private

30e7790

Add private comment before writeln to avoid any interference

49835ed

Use a temporary directory to store the writeln transformed Phobos code

2e1e655

Add a soft fallback if no rsync is provided

07da414

Use .generated for Phobos transformation builds

b361d73

wilzbach force-pushed the assert_writeln_magic branch from 9d02049 to b361d73 Compare February 26, 2017 08:13

CyberShadow approved these changes Feb 27, 2017

View reviewed changes

CyberShadow merged commit 18062eb into dlang:master Feb 27, 2017

wilzbach deleted the assert_writeln_magic branch February 27, 2017 15:13

CyberShadow reviewed Feb 27, 2017

View reviewed changes

Uh oh!

Conversation

wilzbach commented Feb 22, 2017

Uh oh!

CyberShadow commented Feb 22, 2017

Uh oh!

wilzbach commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberShadow commented Feb 23, 2017

Uh oh!

CyberShadow commented Feb 23, 2017

Uh oh!

wilzbach commented Feb 23, 2017

Uh oh!

wilzbach commented Feb 23, 2017

Uh oh!

CyberShadow commented Feb 23, 2017

Uh oh!

CyberShadow commented Feb 23, 2017

Uh oh!

wilzbach commented Feb 23, 2017

Uh oh!

CyberShadow commented Feb 23, 2017

Uh oh!

wilzbach commented Feb 24, 2017

Uh oh!

CyberShadow commented Feb 24, 2017

Uh oh!

CyberShadow 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!

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!

wilzbach commented Feb 24, 2017

Uh oh!

CyberShadow commented Feb 24, 2017

Uh oh!

CyberShadow commented Feb 24, 2017

Uh oh!

wilzbach commented Feb 24, 2017

Uh oh!

CyberShadow commented Feb 24, 2017

Uh oh!

wilzbach commented Feb 24, 2017

Uh oh!

Hackerpilot commented Feb 24, 2017

Uh oh!

wilzbach commented Feb 24, 2017

Uh oh!

wilzbach commented Feb 26, 2017

Uh oh!

CyberShadow commented Feb 27, 2017

Uh oh!

wilzbach commented Feb 27, 2017

Uh oh!

CyberShadow commented Feb 27, 2017

Uh oh!

CyberShadow commented Feb 27, 2017

Uh oh!

CyberShadow commented Feb 27, 2017

Uh oh!

wilzbach commented Feb 23, 2017 •

edited

Loading