Skip to content

Commit

Permalink
Do not use temporary files for adoc generation, closes #54
Browse files Browse the repository at this point in the history
  • Loading branch information
@joaompinto
joaompinto committed Mar 26, 2018
1 parent 1a2319b commit b67c4cc
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 40 deletions.
9 changes: 4 additions & 5 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@
"default": 200,
"description": "Maximum size of output buffer from preview command in kB. Increase if you receive a stdout maxBuffer exceeded error"
},
"AsciiDoc.html_generator": {
"AsciiDoc.asciidoctor_binary_path": {
"type": "string",
"default": "asciidoctor -o-",
"description": "command to be used for the HTML generation"
"default": "asciidoctor",
"description": "Full path for the asciidoctor binary/executable"
},
"AsciiDoc.runInterval": {
"type": "number",
Expand Down Expand Up @@ -127,7 +127,6 @@
"dependencies": {
"vscode": "^1.0.0",
"asciidoctor.js": "^1.5.6-preview.5",
"file-url": "^1.0.1",
"tmp": "^0.0.29"
"file-url": "^1.0.1"
}
}
75 changes: 40 additions & 35 deletions src/AsciiDocProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@ import {
} from 'vscode';
import * as Asciidoctor from "asciidoctor.js";

import { exec } from "child_process";
import { spawn } from "child_process";
import * as fs from "fs";
import * as path from "path";
let fileUrl = require("file-url");
let tmp = require("tmp");

const asciidoctor_config = {
runtime: {
platform: 'node',
engine: 'v8'
}
}

export default class AsciiDocProvider implements TextDocumentContentProvider {
static scheme = 'adoc-preview';
Expand All @@ -32,7 +38,9 @@ export default class AsciiDocProvider implements TextDocumentContentProvider {
private needsRebuild : boolean = true;
private editorDocument: TextDocument = null;
private refreshInterval = 1000;
private asciidoctor = Asciidoctor();


private asciidoctor = Asciidoctor(asciidoctor_config);

private resolveDocument(uri: Uri): TextDocument {
const matches = workspace.textDocuments.filter(d => {
Expand Down Expand Up @@ -125,46 +133,43 @@ export default class AsciiDocProvider implements TextDocumentContentProvider {
let use_asciidoctor_js = workspace.getConfiguration('AsciiDoc').get('use_asciidoctor_js');
let text = doc.getText();
let documentPath = path.dirname(doc.fileName);
let tmpobj = doc.isUntitled ? tmp.fileSync({ postfix: '.adoc' }) : tmp.fileSync({ postfix: '.adoc', dir: documentPath });
fs.write(tmpobj.fd, text, 0);


if(use_asciidoctor_js)
{
const options = {safe: 'unsafe', doctype: 'article', header_footer: true, attributes: ['copycss'], to_file: false};
const options = {
safe: 'unsafe',
doctype: 'article',
header_footer: true,
attributes: ['copycss'],
to_file: false,
base_dir: path.dirname(doc.fileName),
sourcemap: true
};

return new Promise<string>((resolve, reject) => {
let resultHTML = this.asciidoctor.convertFile(tmpobj.name, options);

tmpobj.removeCallback();
let result = this.fixLinks(resultHTML, doc.fileName);
resolve(this.buildPage(result));
let resultHTML = this.asciidoctor.convert(text, options);
//let result = this.fixLinks(resultHTML, doc.fileName);
resolve(this.buildPage(resultHTML));
})
} else
return new Promise<string>((resolve, reject) => {
let html_generator = workspace.getConfiguration('AsciiDoc').get('html_generator')
let cmd = `${html_generator} "${tmpobj.name}"`
let maxBuff = parseInt(workspace.getConfiguration('AsciiDoc').get('buffer_size_kB'))
exec(cmd, {maxBuffer: 1024 * maxBuff}, (error, stdout, stderr) => {
tmpobj.removeCallback();
if (error) {
let errorMessage = [
error.name,
error.message,
error.stack,
"",
stderr.toString()
].join("\n");
console.error(errorMessage);
errorMessage = errorMessage.replace("\n", '<br><br>');
errorMessage += "<br><br>"
errorMessage += "<b>If the asciidoctor binary is not in your PATH, you can set the full path.<br>"
errorMessage += "Go to `File -> Preferences -> User settings` and adjust the AsciiDoc.html_generator config option.</b>"
errorMessage += "<br><br><b>Alternatively if you get a stdout maxBuffer exceeded error, Go to `File -> Preferences -> User settings and adjust the AsciiDoc.buffer_size_kB to a larger number (default is 200 kB).</b>"
resolve(this.errorSnippet(errorMessage));
} else {
let result = this.fixLinks(stdout.toString(), doc.fileName);
resolve(this.buildPage(result));
}
let asciidoctor_binary_path = workspace.getConfiguration('AsciiDoc').get('asciidoctor_binary_path');
const asciidoctor = spawn('asciidoctor', ['-o-', '-', '-B', path.dirname(doc.fileName)]);
asciidoctor.stdin.write(text);
asciidoctor.stdin.end();
asciidoctor.stderr.on('data', (data) => {
let errorMessage = data.toString();
console.error(errorMessage);
errorMessage += errorMessage.replace("\n", '<br><br>');
errorMessage += "<br><br>"
errorMessage += "<b>If the asciidoctor binary is not in your PATH, you can set the full path.<br>"
errorMessage += "Go to `File -> Preferences -> User settings` and adjust the AsciiDoc.asciidoctor_binary_path/b>"
resolve(this.errorSnippet(errorMessage));
})
asciidoctor.stdout.on('data', (data) => {
let result = this.fixLinks(data.toString(), doc.fileName);
resolve(this.buildPage(result));
});
});
}
Expand Down
1 change: 1 addition & 0 deletions test/samples/footer.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Footer text
4 changes: 4 additions & 0 deletions test/samples/script.pl
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
$num_words = "eight";
print "There are ";
print $num_words;
print " words altogether in this sentence.\n";

18 comments on commit b67c4cc

@danyill
Copy link
Contributor

@danyill danyill commented on b67c4cc Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW 'asciidoctor_binary_path' is not obviously being used although defined in L157 above, even though I think L158 should work fine (I cannot see the problem I am experiencing in #33)

Hmmm... Maybe I can. Partially. The white screen is what I get when I receive a clean exit from Asciidoctor. Sometimes I overload the same ids in a document and in the "long document" I checked I also had an Asciidoctor warning (even though I sloppily did not mention it):

asciidoctor: WARNING: logic_requirements_include.adoc: line 23: id assigned to block already in use: processing_capacity asciidoctor: WARNING: logic_requirements_include.adoc: line 23: id assigned to block already in use: processing_capacity

Sometimes Asciidoctor (at least partially) misreports errors. These warnings are actually valid.

I don't think it is safe to assume any output on stderr is actually an error (see here) instead it may be diagnostic or even informative. In general I don't think this is a safe assumption on *nix based systems. I know that seems counterintuitive...

Either way I don't think we want WARNING to prevent preview generation.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I see that previously we were not relying on stderr but getting a more direct indication from asciidoctor of an error. I suggest we try to reinstate this.

@joaompinto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usign spawn() with the default shell: false instead of exec() was causing an unhandled exception (file not found for the default "asciidoctor" path), this prevented the error from being shown in the html. Should be fixed by 43faf6e .

I have tested by setting the wrong binary path, seems to be fine, but please let me know if it worked for your error case.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I do not know why and will look into this further at another time but the problem is if a stylesheet is set within an asciidoc file the user gets a "white screen" problem with no errors and no document in the DOM. However when debugging the code the information is all there. It appears it is not handled correctly by VS Code.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

All you need to test this is the default stylesheet (asciidoctor.css) and a simple document like:

= Just a simple test
:stylesheet: asciidoctor.css

Hey how you doing?

Removing the stylesheet directive causes the preview to function correctly.
In the previous code this did not occur.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding stderr. Perhaps that's OK to use if we also pass -q, or --quiet to silence warnings.

@joaompinto
Copy link
Contributor Author

@joaompinto joaompinto commented on b67c4cc Mar 27, 2018 via email

Choose a reason for hiding this comment

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

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

So no blank screen for you with the attached?

Test.zip

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, something weird about the direct import of fontawesome here which I'll look into further. I think -q is a good idea. At the end of the day this code seems to be fine.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Here comes the bad news.

I think there's (yet another!) buffer limit.

Explanation:

  1. My stylesheet is quite large because I've embedded e.g. FontAwesome.
  2. I've cut it down bit by bit and manage to produce a partially processed Asciidoc file.
  3. The "normal" blank screen is because the buffer is terminated before it even gets near the output of the html (it's way back in CSS declarations still) so the VS Code output is just a pile of CSS and no human readable text.

The following issue suggests there's a 200 kB limit:

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a very good explanation above, so I keep meandering.

On about L93-L97 I did:

    private buildPage(document: string): string {
        // console.log(document);
        console.log((Buffer.byteLength(document, 'utf8').toString()));
        return document;
    }

And then at about L180:

                asciidoctor.stdout.on('data', (data) => {
                    let result = this.fixLinks(data.toString(), doc.fileName);
                    // console.log(data.toString());
                    console.log("spawn: " + Buffer.byteLength(data.toString(), 'utf8').toString());
                    resolve(this.buildPage(result));
                });

This gives me a debug output of:

spawn: 65536
AsciiDocProvider.js:133
65536
AsciiDocProvider.js:68
spawn: 4651
AsciiDocProvider.js:133
4651

That number 65536 feels like some kind of limit.
I don't really understand JS promises/resolve so I am hoping this makes more sense to you than it does to me 😄

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand the problem but my JS is too weak to fix it.

The promise should only be resolved when the process is finished. So we should have something like:

     return new Promise<string>((resolve, reject) => {
                let asciidoctor_binary_path = workspace.getConfiguration('AsciiDoc').get('asciidoctor_binary_path', 'asciidoctor');
                var options = { shell: true, cwd: path.dirname(doc.fileName)}
                var asciidoctor = spawn(asciidoctor_binary_path, ['-q', '-o-', '-', '-B', path.dirname(doc.fileName)], options );
                asciidoctor.stdin.write(text);
                asciidoctor.stdin.end();

                var result = "";
                
                asciidoctor.stderr.on('data', (data) => {
                    let errorMessage = data.toString();
                    console.error(errorMessage);
                    errorMessage += errorMessage.replace("\n", '<br><br>');
                    errorMessage += "<br><br>"
                    errorMessage += "<b>If the asciidoctor binary is not in your PATH, you can set the full path.<br>"
                    errorMessage += "Go to `File -> Preferences -> User settings` and adjust the AsciiDoc.asciidoctor_binary_path/b>"
                    //resolve(this.errorSnippet(errorMessage));
                })
                asciidoctor.stdout.on('data', (data) => {
                    console.log(data.length);
                    result += data.toString();
                    console.log('R' + result.length.toString());
                    //let result2    = this.fixLinks(data.toString(), doc.fileName);
                    //console.log(data.toString());
                    //console.log("spawn: " + Buffer.byteLength(data.toString(), 'utf8').toString());
                    //result = result + result2
                });

                asciidoctor.on('exit', function (code) {
                    // console.log("Tada:" + result.toString());
                    console.log('exit');
                    console.log(result.length.toString());
                    result = this.fixLinks(result.toString(), doc.fileName)
                    console.log(result.length.toString());
                    resolve(this.buildPage(result));
                    console.log("Resolved?");
                  });
                
            });
    }

I think the current implementation stops after the first 64K chunk on stdout comes through. Unfortunately in the above code the resolve(this.buildPage(result)); is not working for mysterious reasons.

@joaompinto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The good news, I can reproduce the problem with the attached zip adoc.
The bad news, I have moved the write() call after the stdout.on() call which would prevent the issue nodejs/node#4236 .

I will do some more research, If we don't find a workaround I will revert to the exec()/buffer setting for the ruby asciidoc option. If your debug output was from a single run, it's an easy fix, it means stdout.on is called multiple times because the output is too big, and we need to concatenate the results :)

@danyill
Copy link
Contributor

@danyill danyill commented on b67c4cc Mar 27, 2018 via email

Choose a reason for hiding this comment

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

@joaompinto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have applied the fix with 503d8c1 but it leaves several questions. I am also very new to nodejs/asynchronous model programming, I am not sure how to deal with the multiple events being delivered with content pieces, and how they "compose" the content with the resolve() call.
Need to read more :(

@joaompinto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading again https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options , and noticing the 'close' event example I was able to create a new fix that makes sense:
1ca309c

I assume on.stdout can be called multiple times (because of it's async nature). And we must concatenate the results, if we want to gather the data until the 'close' event is received.

So the issue was not the buffer limit per si (that 64k value is probably the asciiidoctor output buffering), but the fact that it was being chunked with multiple on_data calls, as the data is received.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great -- thanks for caring about all these things. I will test this evening or tomorrow morning.

@danyill
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @joaompinto. Works like a charm now. 👍

Please sign in to comment.