Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image lookup does not work if both pdf-themesdir and pdf-theme are set to distinct uri:classloader: paths #2383

Closed
sschuberth opened this issue Dec 30, 2022 · 39 comments
Assignees
Milestone

Comments

@sschuberth
Copy link

sschuberth commented Dec 30, 2022

Can there possibly be some regression @mojavelinux? It seems despite me specifying a pdf-themesdir (as a uri:classpath uri:classloader), the background-image is being prefixed with a local path. This code triggers

Dec 29, 2022 7:50:30 PM uri:classloader:/gems/asciidoctor-pdf-2.3.4/lib/asciidoctor/pdf/converter.rb resolve_background_image
WARNING: title page background image not found or readable: C:/Users/SEBAST~1/AppData/Local/Temp/ort-PdfTemplateReporter-asciidoc8842694912548045678/uri:classloader:/images/ort.png

Hmm, or does pdf-themesdir (in contrast to pdf-fontsdir) not support uri:classpath uri:classloader? At least it's not explicitly mentioned in the docs.

Originally posted by @sschuberth in #1829 (comment)

@sschuberth
Copy link
Author

sschuberth commented Dec 30, 2022

More about my use-case: In my code, I'd like to read all of the theme, images, and fonts from resources in a JAR. In the theme file, I'd like to use paths that are relative to the respective resource path.

This works fine with fonts, where I set pdf-fontsdir to "uri:classloader:/fonts" and use e.g. normal: FiraSans-Regular.ttf in the theme. However, the same does not work for pdf-themesdir: No matter what I do, it seems to always be interpreted as a local file system path. For pdf-theme on the other hand, it again works to point it as a resource like "uri:classloader:/templates/asciidoc/pdf-theme.yml".

@mojavelinux
Copy link
Member

I don't have time to work on this right now. If you can see where in the code the logic needs to be changed and are willing to work on it with tests, I'll do my best to guide you through the steps of getting a PR submitted and merged.

@sschuberth
Copy link
Author

sschuberth commented Dec 30, 2022

Thanks for the reply @mojavelinux. Unfortunately, my Ruby is rusty at best. And after searching for uses of "pdf-fontsdir" (to see how it's handled there) and "uri:classpath" "uri:classloader", I only found hits in "lib/asciidoctor/pdf/converter.rb" that don't look like they implement any relevant logic, so I'm a bit lost. Is this even the right repo to address the issue?

@mojavelinux
Copy link
Member

This is the right project. There are things that need to be done (or at least not done) to support loading resources from the classpath. And there are several tests to verify that this functionality works in Asciidoctor PDF. See

It's possible you have found a case not covered by those tests. But, like I said, I don't have time right now to track it down. If you're not willing or able to debug the scenario, then no change will happen anytime in the near future.

@mojavelinux
Copy link
Member

I only found hits in "lib/asciidoctor/pdf/converter.rb" that don't look like they implement any relevant logic

That's because the relevant logic should be in JRuby itself.

@mojavelinux
Copy link
Member

I think the mistake you've made is that you are using uri:classpath: as the prefix instead of uri:classloader: (at least based on your comment). I have updated the docs to be more clear about what prefix to use. See https://docs.asciidoctor.org/pdf-converter/latest/theme/apply-theme/#theme-and-font-directories. I have also added more tests.

Here's an example (expressed as CLI options even though you can also pass them using the Java API):

-a pdf-themesdir=uri:classloader:/templates/asciidoc -a pdf-theme=name

JRuby will then look for the theme at: uri:classloader:/templates/asciidoc/name-theme.yml.

Note that the -theme.yml suffix is not required when specifying the name of the theme.

@mojavelinux mojavelinux self-assigned this Mar 31, 2023
@mojavelinux mojavelinux added this to the v2.3.x milestone Mar 31, 2023
@mojavelinux
Copy link
Member

Note that if pdf-themesdir is set, it must be everything except the basename of the theme file. It's not possible to specify part of the directory in pdf-themesdir and part of it in pdf-theme.

@sschuberth
Copy link
Author

I think the mistake you've made is that you are using uri:classpath: as the prefix instead of uri:classloader: (at least based on your comment).

Yeah, sorry, I was mixing up "classpath" with "classloader" in my comments, but in my comments only. As you can see from the error message, in my actual code I was using "classloader":

WARNING: title page background image not found or readable: C:/Users/SEBAST~1/AppData/Local/Temp/ort-PdfTemplateReporter-asciidoc8842694912548045678/uri:classloader:/images/ort.png

@mojavelinux
Copy link
Member

Thanks for clarifying.

What I can tell you is that this definitely works. First, I have tests in Asciidoctor PDF to prove it. I have also tested it with the Maven plugin examples and it works there too. That's really all I can do unless you can provide a demo of a case when it is not working. I can't fix code that is already behaving as expected.

@sschuberth
Copy link
Author

FYI, here is the work-around that I'm currently applying:

// Images are being looked up relative to the themes directory. As images currently are the only use for
// the themes directory, point it at the images directory. However, the themes directory does not
// support the "uri:classloader:" syntax and can only refer to local paths, see
// https://github.com/asciidoctor/asciidoctor-pdf/issues/2383. So extract the images resource to the
// temporary directory and point to there.
val imagesDir = outputDir.resolve("images").safeMkdirs()
extractImageResources(imagesDir)
attribute("pdf-themesdir", imagesDir.absolutePath)

"uri:classloader:/templates/asciidoc/pdf-theme.yml"

@sschuberth
Copy link
Author

That's really all I can do unless you can provide a demo of a case when it is not working. I can't fix code that is already behaving as expected.

That's fair. IIRC, strictly speaking the issues was not about pdf-themesdir not supposing classloader, but that if pdf-themesdir is set via classloader, then the lookup of images does not work relative to it.

@mojavelinux
Copy link
Member

What I can tell is is that the lookup of images definitely works in this case. If it does not, then there's something else going on with your environment. We have tests in Asciidoctor PDF that run on Linux and Windows to prove that it's absolutely supported regardless of whether you use pdf-theme or pdf-themesdir.

@mojavelinux
Copy link
Member

mojavelinux commented Mar 31, 2023

If you prefer to set the pdf-theme directly because it works better for your environment, that's fine. But please don't state that Asciidoctor PDF doesn't support using pdf-themesdir with uri:classloader: since it absolutely does support it.

Btw, I'm happy to be proven wrong, but you actually have to demonstrate the failure in an isolated environment that I can actually debug.

@sschuberth
Copy link
Author

sschuberth commented Apr 1, 2023

It's not exactly a minimal example, but this change triggers the behavior I'm seeing, and running our PdfTemplateReporterFunTest then fails with

Apr 01, 2023 5:56:30 PM uri:classloader:/gems/asciidoctor-pdf-2.3.4/lib/asciidoctor/pdf/converter.rb convert_image
WARNING: image to embed not found or not readable: C:/Users/SEBAST~1/AppData/Local/Temp/ort-PdfTemplateReporter-asciidoc7356378083635247542/uri:classloader:/images/ort.png

Before going deeper into this, let's check if my expectations are correct. Given:

  • I'm setting pdf-themesdir to uri:classloader:/images
  • I'm also setting pdf-theme to uri:classloader:/templates/asciidoc/pdf-theme.yml
  • The pdf-theme.yml resource refers to the title page logo via image: image:ort.png[pdfwidth=80%]

I'd expect that ort.png is being looked up relative to the /images resource directory. Is my expectation valid?

@sschuberth sschuberth changed the title Please support uri:classpath: syntax also for pdf-themesdir Title page logo image is not being looked up relative to pdf-themesdir if the latter start with uri:classloader: Apr 1, 2023
@mojavelinux
Copy link
Member

mojavelinux commented Apr 1, 2023

  • I'm setting pdf-themesdir to uri:classloader:/images
  • I'm also setting pdf-theme to uri:classloader:/templates/asciidoc/pdf-theme.yml

There's the problem right there. pdf-themesdir is not a general purpose attribute. It must resolve to the directory that contains the theme yml file. If you attempt to make it different, strange things will happen.

The reason it works when you specify pdf-theme only is because pdf-themesdir is then computed to be the directory of that file.

@sschuberth
Copy link
Author

pdf-themesdir is not a general purpose attribute. It must resolve to the directory that contains the theme yml file.

Thanks for clarifying, that wasn't obvious to me from reading the docs. Would it make sense to fail early then, with a dedicated error message if pdf-themesdir is not a parent of pdf-theme?

If you attempt to make it different, strange things will happen.

Is this by design, or would it make sense to relax the entanglement between pdf-themesdir and pdf-theme to allow them to be set completely independently?

@mojavelinux
Copy link
Member

mojavelinux commented Apr 2, 2023 via email

@sschuberth sschuberth changed the title Title page logo image is not being looked up relative to pdf-themesdir if the latter start with uri:classloader: Image lookup does not work if both pdf-themesdir and pdf-theme are set to distinct uri:classloader: paths Apr 2, 2023
@sschuberth
Copy link
Author

Ok, I gave it a new try:

  • pdf-themesdir is set to "uri:classloader:/pdf-theme" (and this resources directory contains both "pdf-theme.yml" and "ort.png"
  • pdf-theme is set to "pdf-theme.yml"
  • "pdf-theme.yml" refers to image: image:ort.png[pdfwidth=80%] as a title page logo

Is that supposed to work? As I'm still getting

WARNING: image to embed not found or not readable: C:/Users/SEBAST~1/AppData/Local/Temp/ort-PdfTemplateReporter-asciidoc662513518992088845/uri:classloader:/pdf-theme/ort.png

Interestingly, it also doesn't work if I just set pdf-theme to "uri:classloader:/pdf-theme/pdf-theme.yml" to let Asciidoctor deduce the pdf-themesdir. I'll make some more tests.

@sschuberth
Copy link
Author

sschuberth commented Apr 2, 2023

I wonder whether asciidoctor/asciidoctor#3929 is related, as it indeed seems like "uri:classloader:/pdf-theme/ort.png" is being treated as a relative path as it gets appended to "C:/Users/SEBAST~1/AppData/Local/Temp/ort-PdfTemplateReporter-asciidoc662513518992088845/" (and yes, I am running on JRuby).

@mojavelinux
Copy link
Member

mojavelinux commented Apr 2, 2023 via email

@mojavelinux
Copy link
Member

Aside from sharing a sample project that isolates your scenario, the only thing I can advise is to join the project chat at https://chat.asciidoctor.org to see if someone else can help you figure out why it doesn't work in your environment. I'm beyond frustrated at this point that the response I keep getting is "sorry, it doesn't work" with no additional details when I've proven time and again that this case does work. I just don't have time to keep posting the same point over and over again on this issue.

@sschuberth
Copy link
Author

If you want any movement on this issue, I need an isolated and reproducible test case in code.

Of course. I'm a maintainer myself and I know how annoying bad issue reports can be. I'm just trying to understand where to start in the mix of my expectations of how things are supposed to work (after reading the docs), Ruby, JRuby, Asciidoctor, Asciidoctor-PDF, etc.

Otherwise, I have to assume there is a problem with your setup or the version of JRuby you're using.

Which might very well be.

I'm beyond frustrated at this point that the response I keep getting is "sorry, it doesn't work" with no additional details

Please don't be frustrated, and don't take my "sorry, it doesn't work" (for me) as equals to blaming you. I really don't. I'm still just trying to understand in which area providing more detail is most meaningful (see above).

I just don't have time to keep posting the same point over and over again on this issue.

That's understandable. Let's settle this issue for good until I've managed to isolate it.

In any case, thanks for your responses so far.

@mojavelinux
Copy link
Member

mojavelinux commented Apr 2, 2023

Note that if pdf-themesdir is set, it must be everything except the basename of the theme file. It's not possible to specify part of the directory in pdf-themesdir and part of it in pdf-theme.

I did some more digging and this turns out to be incorrect. It is possible to use a directory other than the directory in which the theme file is located. (When using the classloader, the paths must not exit the classloader, but that probably goes without saying). I've updated the docs to include a more detailed explanation of how paths are resolved: https://docs.asciidoctor.org/pdf-converter/latest/theme/apply-theme/#theme-and-font-directories

And, to be clear, I've tested this manually (in addition to the automated tests) with Asciidoctor PDF on CRuby, Asciidoctor PDF on JRuby, AsciidoctorJ PDF with the Maven plugin, and AsciidoctorJ PDF using the asciidoctorj bin script (installed via SDKman). In all cases, it works exactly as I have described it in the documentation.

@sschuberth
Copy link
Author

Ok, so after some lengthy debugging session I finally found the root cause of my issue that I'd like to share. It's caused by an implicit update of the JRuby version used by AsciidoctorJ:

+--- org.asciidoctor:asciidoctorj:2.5.7
|    +--- org.asciidoctor:asciidoctorj-api:2.5.7
|    \--- org.jruby:jruby:9.3.8.0 -> 9.4.2.0

As you can see, JRuby 9.3.8.0 gets updated to version 9.4.2.0 by Gradle (due to version 9.4.2.0 already being used by other code in the same project). This is what triggers

WARNING: image to embed not found or not readable: C:/Users/Sebastian/Development/GitHub/sschuberth/asciidoctorj-pdf-template-uri-classloader-issue/app/src/main/resources/templates/asciidoc/uri:classloader:/pdf-theme/ort.png

Here is an example project that demonstrates the issue.

Sorry @mojavelinux for the noise after all, and thanks a lot for bearing with me.

@mojavelinux
Copy link
Member

Thanks for conducting that research and sharing your findings. This looks like it boils down to a regression in JRuby, perhaps even specific to Windows. I wonder if it would be possible to go one step further and trigger it using JRuby itself, without Asciidoctor involved.

@sschuberth
Copy link
Author

This looks like it boils down to a regression in JRuby, perhaps even specific to Windows.

I can actually confirm it to happen on Linux, too.

I wonder if it would be possible to go one step further and trigger it using JRuby itself, without Asciidoctor involved.

Would you have any hint how to do that? Like triggering a resource lookup that's supposed to be relative to another resource directory?

Alternatively, any hints how to try with an AsciidoctorJ pre-release, as that seems to have been upgraded to JRuby 9.4.1.0?

@mojavelinux
Copy link
Member

It should be as simple as calling File.absolute_path (or similar) on a uri:classloader/ path. You're looking at any methods that are called in this region of the code: https://github.com/asciidoctor/asciidoctor-pdf/blob/main/lib/asciidoctor/pdf/theme_loader.rb#L49-L64

@mojavelinux
Copy link
Member

I can actually confirm it to happen on Linux, too.

I'm very frustrated with JRuby that this regression was allowed to happen. I just can't see how this bug made it through the test suite and into a release.

@mojavelinux
Copy link
Member

Out of curiosity, I ran the Asciidoctor PDF test suite using JRuby 9.4.2.0. I can confirm that even our tests fail. So either JRuby has a good reason for changing this behavior (and we have to rethink our assumptions), or it's a bug in JRuby.

@sschuberth
Copy link
Author

sschuberth commented Apr 5, 2023

I can confirm that even our tests fail.

Thanks for checking. It's somewhat relieving to not be alone 😉

So either JRuby has a good reason for changing this behavior (and we have to rethink our assumptions), or it's a bug in JRuby.

I wasn't able to find a hint about such a behavior change in JRuby's release notes. So I'll probably file a bug with JRuby and see what the responses are.

@mojavelinux
Copy link
Member

I found the exact change. Consider the following Ruby script:

require 'pathname'

p (::Pathname.new 'uri:classloader:/pdf-themes/logo.png').absolute?
if File.respond_to? :absolute_path?
  p File.absolute_path? 'uri:classloader:/pdf-themes/logo.png'
end

Here are the results with different versions of JRuby:

  • JRuby 9.4.2.0 - true, false
  • JRuby 9.3.7.0 - true
  • JRuby 9.1.17.0 - true

What happened is that JRuby 9.4.2.0 implemented File.absolute_path? for the first time, but its return value does not match Pathname#absolute?. That breaks the following assumption in Asciidoctor PDF: https://github.com/asciidoctor/asciidoctor-pdf/blob/main/lib/asciidoctor/pdf/ext/core/file.rb

That means we can't rely on it. I can put in a workaround in the meantime, but this really strikes me as a bug in JRuby. I don't see why these two return values should be different.

@mojavelinux
Copy link
Member

mojavelinux commented Apr 5, 2023

I just learned that JRuby 9.4.2.0 also gets the result wrong for a URL.

Consider this code:

require 'pathname'
p (Pathname.new 'http://192.168.1.13:9876/bg.png').absolute?

That should be false, but as of JRuby 9.4.2.0 it is true. (This is absolutely not compliant with CRuby).

I'm quickly losing faith in the JRuby test suite if it gets this kind of stuff wrong.

@sschuberth
Copy link
Author

this really strikes me as a bug in JRuby.

Are you going to report it, or should I? (I guess you have more creds in that community than me, so I'd appreciate if you did.)

@mojavelinux
Copy link
Member

I don't have time to track this down right now. But I am trying to get a patch into Asciidoctor PDF to work around it since this has the potential for causing a lot of problems for people upgrading.

@sschuberth
Copy link
Author

I've created jruby/jruby#7744 to raise awareness with the JRuby folks.

@enebo
Copy link

enebo commented Apr 5, 2023

@mojavelinux in Ruby 3.1 (JRuby 9.4.x) we stopped using our own pathname and started using a standardized gem. Something got lost in transit. The File#absolute_path? is a new method which just followed C Ruby's logic (e.g. I forgot about this value-added part). Opened jruby/jruby#7745 for that.

@mojavelinux
Copy link
Member

Thanks for the insight @enebo! I'm glad to see that you are now tracking this in JRuby.

One odd thing I noticed is that File.absolute_path? returns true for a regular URL, which seems really odd (and certainly breaks assumptions we've been relying on in Asciidoctor). Here's the workaround I've put in place in the meantime: https://github.com/asciidoctor/asciidoctor-pdf/blob/main/lib/asciidoctor/pdf/ext/core/file.rb#L6

@enebo
Copy link

enebo commented Apr 6, 2023

@mojavelinux I have not stepped through this yet but that is baffling to me too. There are specs to test absolute_path? but because I forgot this should have added our value-added logic for URIs it just calls the most obvious methods (and the normal ruby specs for this pass). When I figure this out we will definitely add tests for this and probably audit what else is calling the methods which made this return true.

@mojavelinux
Copy link
Member

@sschuberth The workaround is included in the 2.3.6 release. I'll probably keep it in the 2.3.x release line, but remove it from main once we can confirm that JRuby handles the logic.

sschuberth added a commit to oss-review-toolkit/ort that referenced this issue Apr 10, 2023
Asciidoctor(J)-PDF 2.3.6 [1] includes a work-around for a bug in JRuby
9.4 that causes `uri:classloader:` paths to be recognized as absolute
paths. Thanks to this work-around the current code can be simplified to
avoid extracting image resources.

However, as explained at [2] the `pdf-themesdir` "must resolve to the
directory that contains the theme yml file". So move both the image and
PDF theme file to a "pdf-theme" resource directory to have a conforming
resource layout. And instead of setting `pdf-themesdir` explicitly, set
it implicitly only by setting `pdf-theme` "because `pdf-themesdir` is then
computed to be the directory of that file."

[1]: https://github.com/asciidoctor/asciidoctor-pdf/releases/tag/v2.3.6
[2]: asciidoctor/asciidoctor-pdf#2383 (comment)

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
sschuberth added a commit to oss-review-toolkit/ort that referenced this issue Apr 10, 2023
Asciidoctor(J)-PDF 2.3.6 [1] includes a work-around for a bug in JRuby
9.4 that causes `uri:classloader:` paths to be recognized as absolute
paths. Thanks to this work-around the current code can be simplified to
avoid extracting image resources.

However, as explained at [2] the `pdf-themesdir` "must resolve to the
directory that contains the theme yml file". So move both the image and
PDF theme file to a "pdf-theme" resource directory to have a conforming
resource layout. And instead of setting `pdf-themesdir` explicitly, set
it implicitly only by setting `pdf-theme` "because `pdf-themesdir` is then
computed to be the directory of that file."

[1]: https://github.com/asciidoctor/asciidoctor-pdf/releases/tag/v2.3.6
[2]: asciidoctor/asciidoctor-pdf#2383 (comment)

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
sschuberth added a commit to oss-review-toolkit/ort that referenced this issue Apr 11, 2023
Asciidoctor(J)-PDF 2.3.6 [1] includes a work-around for a bug in JRuby
9.4 that causes `uri:classloader:` paths to be recognized as absolute
paths. Thanks to this work-around the current code can be simplified to
avoid extracting image resources.

However, as explained at [2] the `pdf-themesdir` "must resolve to the
directory that contains the theme yml file". So move both the image and
PDF theme file to a "pdf-theme" resource directory to have a conforming
resource layout. And instead of setting `pdf-themesdir` explicitly, set
it implicitly only by setting `pdf-theme` "because `pdf-themesdir` is then
computed to be the directory of that file."

[1]: https://github.com/asciidoctor/asciidoctor-pdf/releases/tag/v2.3.6
[2]: asciidoctor/asciidoctor-pdf#2383 (comment)

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants