asciidoctor / asciidoctorj

:coffee: Java bindings for Asciidoctor. Asciidoctor on the JVM!
http://asciidoctor.org
Apache License 2.0
627 stars 172 forks source link

Exception when using 1.6.0-alpha.1 with custom templates #417

Open nawroth opened 8 years ago

nawroth commented 8 years ago
Caused by: java.lang.ClassCastException: org.jruby.RubyObject cannot be cast to java.lang.String
    at org.asciidoctor.internal.JRubyAsciidoctor.convertFile(JRubyAsciidoctor.java:501)
    at org.asciidoctor.internal.JRubyAsciidoctor.renderFile(JRubyAsciidoctor.java:255)
    at org.asciidoctor.internal.JRubyAsciidoctor.renderAllFiles(JRubyAsciidoctor.java:309)
    at org.asciidoctor.internal.JRubyAsciidoctor.renderFiles(JRubyAsciidoctor.java:285)
    at org.asciidoctor.internal.JRubyAsciidoctor.renderFiles(JRubyAsciidoctor.java:292)
    at org.asciidoctor.internal.JRubyAsciidoctor.convertFiles(JRubyAsciidoctor.java:578)
    at org.asciidoctor.Asciidoctor$convertFiles.call(Unknown Source)

AsciidoctorJ is called from Gradle if that matters. (Plain Gradle/Groovy, not the plugin.) You can try out the failing build from https://github.com/nawroth/browser-content/tree/1.6.0-alpha.1 It works fine with 1.5.4.

mojavelinux commented 8 years ago

This seems to only happen when templates are used, but the error is not coming from the template itself.

Of course, the error itself is not helpful, so debugging is probably required to figure out which object it is referring to.

I'm curious, why aren't you using the Gradle plugin? It seems like you are just reinventing the wheel with the custom Gradle file?

Also, why ERB? Why? ERB is so unbelievably painful to work with. I strongly recommend using Slim or Haml instead. Then you can actually make sense of the template.

mojavelinux commented 8 years ago

I'd say the template converter is broken in AsciidoctorJ 1.6.0 somehow. Surely there must be a test for this. As soon as I pass a template directory that has at least one file in it, boom.

mojavelinux commented 8 years ago

One of the things the Gradle plugin does for you is ensure that all GStrings have been resolved so that the Java API only sees regular strings. There are numerous other benefits it provides as well that you'll have to solve over again by using a homegrown solution.

...either way, we still seem to have a problem with templates in AsciidoctorJ 1.6.0.

nawroth commented 8 years ago

This was just a quick proof of concept, inheriting its build layout from https://github.com/neo4j/driver-documentation where we couldn't use the Gradle plugin as we needed multiple Asciidoctor executions with distinct configs. We would need something along the lines of https://github.com/asciidoctor/asciidoctor-gradle-plugin/issues/161 I spent too much time trying to get around the limitations of the plugin as I really didn't want a homegrown solution ;-)

Using plain Gradle was tricky at first but gave us the level of control we needed. It's quite cool how Groovy suggests solutions to problems, I think that's how we found out how to deal with the strings.

We started out with Slim - it took a while to find out it wasn't supported in 1.5.x. And then we got stuck with what I reported above. As there already existed ERB templates for what we wanted to do we went with those.

mojavelinux commented 8 years ago

we couldn't use the Gradle plugin as we needed multiple Asciidoctor executions with distinct configs

The Gradle plugin supports this use case. For an example, see https://github.com/mraible/infoq-mini-book/blob/master/build.gradle. Unlike what this example shows, you can use the same converter across different tasks.

mojavelinux commented 8 years ago

it took a while to find out it wasn't supported in 1.5.x.

That's news to me. Why doesn't it work in 1.5.x? (If Slim is causing trouble for some reason, just use Haml which is 98% the same).

mojavelinux commented 8 years ago

As there already existed ERB templates for what we wanted to do we went with those.

It's really worth the effort to run away screaming from ERB. I spent many, many nights making ERB templates for asciidoctor-backends and I never wanted to do it again. It is way too stressful to hold the parse stack in your head, as you have to do to understand ERB.

mojavelinux commented 8 years ago

I updated the title of the issue to reflect what I think the issue is. I'd like to see a test passing, because right now I'm not believing that it works.

robertpanzer commented 8 years ago

Well we actually have unit tests for custom templates with slim and haml, both in master and in the asciidoctor-1.6.0 branch:

There's a change in the AsciidoctorJ 1.6.0 branch where convertFile(filename, options) expects that Asciidoctor core returns a String or null and casts the result from the Asciidoctor call to a String. That's where the ClassCastException occurs.

Changing the build task to render all files indiviually and expecting any result makes the build (nearly) pass for me:

    tree.files.each {
        getAsciidoctor().convertFile(it, opts.get(), Object.class)
    }

Strange thing is that Asciidoctor returns the Asciidoctor::Document Ruby object when templates are used, null when the template is removed. Strange that this does not occur in our unit tests, but I will dig into it in the next days.

Nearly pass because I get exception for the two files guides/northwind-graph.adoc and help/query-plan.adoc:

Caused by: org.jruby.exceptions.RaiseException: (Encoding::CompatibilityError) incompatible encodings: UTF-8 and Windows-1252
        at org.jruby.RubyArray.join(org/jruby/RubyArray.java:1783)
        at org.jruby.RubyArray.*(org/jruby/RubyArray.java:2960)
        at uri_3a_classloader_3a_.gems.asciidoctor_minus_1_dot_5_dot_3.lib.asciidoctor.abstract_block.content(uri:classloader:/gems/asciidoctor-1.5.3/lib/asciidoctor/abstract_block.rb:80)
        at RUBY.block in singleton class(C:/work/git/browser-content/src/main/resources/erb/section.html.erb:8)
        at org.jruby.RubyBasicObject.instance_eval(org/jruby/RubyBasicObject.java:1633)
        at RUBY.singleton class(C:/work/git/browser-content/src/main/resources/erb/section.html.erb:1)
        at RUBY.__tilt_2020(C:/work/git/browser-content/src/main/resources/erb/section.html.erb:1)
        at org.jruby.RubyMethod.call(org/jruby/RubyMethod.java:115)
        at uri_3a_classloader_3a_.gems.tilt_minus_2_dot_0_dot_1.lib.tilt.template.evaluate(uri:classloader:/gems/tilt-2.0.1/lib/tilt/template.rb:155)
        at uri_3a_classloader_3a_.gems.tilt_minus_2_dot_0_dot_1.lib.tilt.template.render(uri:classloader:/gems/tilt-2.0.1/lib/tilt/template.rb:96)
        at uri_3a_classloader_3a_.gems.asciidoctor_minus_1_dot_5_dot_3.lib.asciidoctor.converter.template.convert(uri:classloader:/gems/asciidoctor-1.5.3/lib/asciidoctor/converter/template.rb:193)
        at uri_3a_classloader_3a_.gems.asciidoctor_minus_1_dot_5_dot_3.lib.asciidoctor.converter.composite.convert(uri:classloader:/gems/asciidoctor-1.5.3/lib/asciidoctor/converter/composite.rb:31)
        at uri_3a_classloader_3a_.gems.asciidoctor_minus_1_dot_5_dot_3.lib.asciidoctor.abstract_block.convert(uri:classloader:/gems/asciidoctor-1.5.3/lib/asciidoctor/abstract_block.rb:71)
nawroth commented 8 years ago

@mojavelinux Oh, AsciidoctorTask isn't mentioned here: https://github.com/asciidoctor/asciidoctor-gradle-plugin Using that should solve https://github.com/asciidoctor/asciidoctor-gradle-plugin/issues/157

We though our problems with Slim came from this: https://github.com/asciidoctor/asciidoctorj/issues/307 which is fixed only in 1.6.0 AFAIK.

@robertpanzer Note that most of the adoc files are generated from HTML using pandoc, so they may be broken in some ways. At the same time, that shouldn't differ when executing Asciidoctor in different ways.

robertpanzer commented 8 years ago

OK, got one step further. The code usually handles when instances Asciidoctor::Document are returned: https://github.com/asciidoctor/asciidoctorj/blob/asciidoctorj-1.6.0/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyAsciidoctor.java#L521

But somehow the match seems to fail. I don't know why, but I'd like to see if the problem persists when using the Asciidoctor Gradle plugin.

At the moment I am more inclined to say that this problem is connected to the custom Asciidoctor integration into Gradle.

mojavelinux commented 8 years ago

Thanks for digging into this @robertpanzer!

I'm glad to see that we have tests that cover it already and also glad that we uncovered another possible scenario to be tested. More tests FTW!

Strange thing is that Asciidoctor returns the Asciidoctor::Document Ruby object when templates are used, null when the template is removed.

That is strange. A quick test on Asciidoctor core and I always get a Document returned. I wonder what is triggering this.

Nearly pass because I get exception for the two files guides/northwind-graph.adoc and help/query-plan.adoc:

I don't get exceptions on these. Everything converts fine on my system with your proposed change. Perhaps it's the default charset on Windows problem. Hard to say.

mojavelinux commented 8 years ago

@nawroth I think #157 is about defining a DSL for doing this more easily. The tasks approach is always available, though I agree it definitely needs to be documented.

https://github.com/asciidoctor/asciidoctorj/issues/307 is very specific to the asciidoctor-backends and only affected one or two templates. Slim works just fine with all versions of AsciidoctorJ. You'd just need to be aware of that very minor syntax change that may affect migration of the templates...but honestly I doubt you'd even hit that change.

At the same time, that shouldn't differ when executing Asciidoctor in different ways.

I don't think he's saying it is. I think it was just the next error he hit after fixing the first problem.

From which format are you generating the files using pandoc? Personally, I don't like the AsciiDoc that pandoc produces. It's old and crufty and wrong in a lot of ways. Then again, you can always clean it up...so if it gets the job done.

mojavelinux commented 8 years ago

But somehow the match seems to fail.

Ah, that makes a little bit more sense.

robertpanzer commented 8 years ago

The match for the Asciidoctor::Document class is done here: https://github.com/asciidoctor/asciidoctorj/blob/asciidoctorj-1.6.0/asciidoctorj-core/src/main/java/org/asciidoctor/ast/NodeConverter.java#L111-L114

Do you think it would make more sense to do a string comparison of the class names? I am actually not really a fan of doing so because either

mojavelinux commented 8 years ago

Aha! I think I may know why it is not failing for you when you try it with the tests. It looks like you changed the logic after alpha.1 was released. It used to use ==, which is probably why it isn't matching. See https://github.com/asciidoctor/asciidoctorj/blob/1.6.0-alpha.1/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyAsciidoctor.java#L448.

robertpanzer commented 8 years ago

Hero!!

Will check this after my current meeting.

Am Freitag, 8. Januar 2016 schrieb Dan Allen :

Aha! I think I may know why it is not failing for you when you try it with the tests. It looks like you changed the logic after alpha.1 was released. It used to use ==, which is probably why it isn't matching. See https://github.com/asciidoctor/asciidoctorj/blob/1.6.0-alpha.1/asciidoctorj-core/src/main/java/org/asciidoctor/internal/JRubyAsciidoctor.java#L448 .

— Reply to this email directly or view it on GitHub https://github.com/asciidoctor/asciidoctorj/issues/417#issuecomment-169982486 .

mojavelinux commented 8 years ago

So basically, the conclusion is, use 1.6.0-SNAPSHOT and the original build works unmodified. This problem was already fixed :)

mojavelinux commented 8 years ago

You might want to add a comment above that line that checks for the Document object to explain when that happens (when Asciidoctor is writing to a file) so we remember why it is there.

mojavelinux commented 8 years ago

Teamwork!

robertpanzer commented 8 years ago

Just checked that it works with the current snapshot, awesome! I will take care of the call stack and the comment in the next days.

Thanks a lot!!!!

robertpanzer commented 8 years ago

You might want to add a comment above that line that checks for the Document object to explain when that happens (when Asciidoctor is writing to a file) so we remember why it is there.

I've already made a comment below the check, does that count too? ;-)

            Object object = this.asciidoctorModule.convert(content, rubyHash);
            if (object instanceof IRubyObject && NodeConverter.NodeType.DOCUMENT_CLASS.isInstance((IRubyObject) object)) {
                // If a document is rendered to a file Asciidoctor returns the document, we return null
                return null;
            }
mojavelinux commented 8 years ago

Oh, good point. Yep, that works.