Upgrade Asciidoctor core to 1.5.5

[robertpanzer]

This PR bumps the version of Asciidoctor to 1.5.5 for the AsciidoctorJ 1.6.x branch.

Additionally it removes the asciidoctorj-pdf module as it lives in its own repo now.
Once this is merged I could release a new 1.6.0-alpha.4 so that there is a 1.6.0 release with the latest and greatest Asciidoctor core released again.

[mojavelinux]

I could definitely see us doing the same for epub3 and diagram. But let's do pdf first and just get used to how that works.

[robertpanzer]

Hmm.... build failed on Java 8.
I will check tomorrow. I also built with java 8 locally on OSX but with an older version (_91)

[robertpanzer]

Seems like there's really a problem with the test.
I couldn't reproduce it locally and for some reason Gradle on Travis thinks it's a bad idea to write out the assertion messages.
Will retry.

[abelsromero]

The failing test is working fine on Windows 8.1 with Java 1.8.0_121.

Edit: 2 test fail, but not really. The PDF test fail as we already know, and 'should render ditaa diagram to HTML', also fails but it's because the image path in the HTML is built with /, but the assertion uses \, but the document is correctly rendered.

[abelsromero]

All fine on MacOS 1.8.0_60, however on Windows 7 with Java 1.8.0_111 I got an error on 'should_be_able_to_read_asset'

org.jruby.exceptions.RaiseException: (LoadError) no such file to load -- asciidoctor

	at org.jruby.RubyKernel.require(org/jruby/RubyKernel.java:961)
	at RUBY.require(uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:55)
	at RUBY.<main>(<script>:9)

[robertpanzer]

Oo...
Didn't try yet on Win, but will also do that.
Is it just an impression or is it really so fragile?

[abelsromero]

Is it just an impression or is it really so fragile?

Being honest, the whole thing of running ruby gems in Java + Java extensions that get hooked into Ruby + compatibility with Java 6, 7, 8 and several OS is quite a feat. So some issues should not be unexpected, however this must never be discouraging.

Back the topic, the only issue that really concerns me is the one with Java 7, I tested more and it only happens from shell, not from IntelliJ....overall...excluding the Travis issue, the others don't worry me enough to block the release:

• The Windows 81. is a path thing, so an assertion that takes that into consideration could be added.
• The PDF issue, is the known one about not being able to find rouge.
• The Windows 7 is odd but the fact that it only happens form shell makes me think could be something with the order of execution or environment get polluted after some tests.

[robertpanzer]

Yes, you're right. Maybe I am just in a pessimistic phase right now ;)

[abelsromero]

Tomorrow I will have a look at the first and third issue in more detail. But still no idea about the Travis issue :/

Maybe we can use this to store the tests reports? https://docs.travis-ci.com/user/uploading-artifacts/

[mojavelinux]

Is it just an impression or is it really so fragile?

Being honest, the whole thing of running ruby gems in Java + Java extensions that get hooked into Ruby + compatibility with Java 6, 7, 8 and several OS is quite a feat.

I think this can be even further simplified. JRuby is not sufficiently tested on Windows. I have yet to see a recurring problem of JRuby running on *nix. Most of the time, the problems on Windows are related to path handling. I think this is a valid criticism of JRuby.

We need to keep in mind that we can only offer as much as what JRuby affords us. In other words, I don't think we should burden ourselves trying to fix problems outside of our own scope. We can do it to a point...but at some point we have to say "we are waiting on upstream to fix that"

[mojavelinux]

I also wonder whether if there's a difference when using Bash vs command vs PowerShell on Windows. So "windows" isn't just one environment.

[ancho]

The jbake gradle plugin is using the following configuration to output what tests are executed and what's going on.

https://github.com/jbake-org/jbake-gradle-plugin/blob/master/build.gradle#L113-L130

I'll prepare a pull request. Maybe that helps debugging the problem.

[ancho]

I can reproduce the failing tests running test-asciidoctor-upstream.sh on my local machine with java 8.

[robertpanzer]

Good point!
I didn't run against upstream.
I'll also try that!
Awesome!

[robertpanzer]

OK, think I have the test errors now.
The initial error is due to a recent fix in the way the data uris are encoded, change from : to ; as a separator.
That's easy to fix so that it builds with both versions.

The new errors come from the renaming of Treeprocessor to TreeProcessor in ::Asciidoctor::Extensions in asciidoctor/asciidoctor@f1dd816.
I'll try to fix it now so that we keep the old Java class name but are able to build and run with Asciidoctor 1.5.5 and 1.5.6.
But I wonder if we also want to rename that class in to TreeProcessor in the AsciidoctorJ 1.6.x branch.
Imo we shouldn't change the name in the Asciidoctor 1.5.x line as we don't have that simple way of aliasing class names in Java and I'm not a fan of having 2 classes Treeprocessor and TreeProcessor in AsciidoctorJ.

[abelsromero]

I noticed AppVeyor is not launching on PRs.
Btw, I can confirm the error with Windows 8 is a problem with the assert. In this change 41a6d5a a full path validation was introduced that is only valid for Unix like systems.

Is as simple as doing this in WhenDitaaDiagramIsRendered

if (Platform.getNativePlatform().OS == Platform.OS.WINDOWS)
    result.contains("""src="${imagesOutDir.absolutePath.replaceAll("\\\\", "//")}/${imageFileName}.png""")
else
    result.contains("""src="${imagesOutDir.absolutePath}/${imageFileName}.png""")

I can send a PR after merge.

[robertpanzer]

Strange, it looks rather that Github doesn't show the build result.
Because Appveyor actually built this PR:
https://ci.appveyor.com/project/asciidoctor/asciidoctorj/build/526

I would be super-happy if you could create a PR to fix this test for Windows.

[mojavelinux]

The new errors come from the renaming of Treeprocessor to TreeProcessor in ::Asciidoctor::Extensions in asciidoctor/asciidoctor@f1dd816.

I'm confused. I already added that alias so it should have 0 impact. Why doesn't AsciidoctorJ see the alias?

https://github.com/asciidoctor/asciidoctor/blob/master/lib/asciidoctor/extensions.rb#L302

[mojavelinux]

But I wonder if we also want to rename that class in to TreeProcessor in the AsciidoctorJ 1.6.x branch.

Yes, this is an official change. We did the same in Asciidoctor.js.

Imo we shouldn't change the name in the Asciidoctor 1.5.x line

👍

[mojavelinux]

Also keep in mind that it's no longer valid to use a backend name that doesn't resolve to a converter. We now check eagerly whether a converter can be resolved. Otherwise, we have the risk of a NilClass exception later on. I think one of the tests was using a bogus backend name, but not checking for an exception early enough.

[mojavelinux]

I'll try to fix (Treeprocessor) now so that we keep the old Java class name but are able to build and run with Asciidoctor 1.5.5 and 1.5.6.

I really want this to work without a change to AsciidoctorJ.

[robertpanzer]

I'm not too happy about this either. If we find another way I would be more than happy, because it's really a hack, but is related to the fact that we support 2 versions of Asciidoctor in every build (latest release plus upstream).
Additionally I'd really like to avoid this name change in AsciidoctorJ 1.5.x.