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

Site generation functionality fails on external image includes #242

Closed
ronaldtse opened this issue May 21, 2021 · 24 comments
Closed

Site generation functionality fails on external image includes #242

ronaldtse opened this issue May 21, 2021 · 24 comments
Assignees
Labels
bug Something isn't working

Comments

@ronaldtse
Copy link
Contributor

In mn-samples-iso, if you remove the :data-uri-image: attribute from one document, you will see a failure like this:

No such file or directory @ rb_sysopen - /Users/me/src/mn/mn-samples-iso/site/documents/iso-rice-en/images/img01.png
/Users/me/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/image_size-2.1.0/lib/image_size.rb:47:in `initialize'
/Users/me/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/image_size-2.1.0/lib/image_size.rb:47:in `open'
/Users/me/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/image_size-2.1.0/lib/image_size.rb:47:in `path'
/Users/me/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/html2doc-1.1.1/lib/html2doc/mime.rb:81:in `image_resize'
/Users/me/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/isodoc-1.6.2/lib/isodoc/html_function/postprocess.rb:150:in `block in move_images'

This is due to the image being external to the file. When resolving any external file, the code should read using a base dir of the original location.

When the :data-uri-image: attribute is used, the external image file is embedded into the XML file, so this error does not occur.

@ronaldtse ronaldtse added the bug Something isn't working label May 21, 2021
@abunashir
Copy link
Member

Hey @ronaldtse, I think this might be coming from the underlying isodoc html function, and how that tries to handle images.

I don't have that much context on that library, so I can't exactly figure out what's happening there, maybe @opoudjis might be able to help us out on that one?

@ronaldtse
Copy link
Contributor Author

The challenge here is when isodoc builds the HTML, it needs to have the XML point out where the images are. e.g. maybe the XML file can store the base image path?

@opoudjis
Copy link
Contributor

The challenge here is when isodoc builds the HTML, it needs to have the XML point out where the images are. e.g. maybe the XML file can store the base image path?

It'd be more constructive to have that passed on to isodoc as a parameter from metanorma, rather than embed something that non-semantic in the semantic source.

@ronaldtse
Copy link
Contributor Author

All you could pass to isodoc is the "base asset path", where isodoc would be expected to resolve the external references. We would need to make that an optional parameter when Metanorma calls the isodoc gem.

@opoudjis
Copy link
Contributor

opoudjis commented Sep 16, 2021

@localdir is the base asset path. It is set currently in isodoc as the parent directory of the filepath passed to isodoc. We want to override that with an option setting, :base-asset-path:, so that the base asset path can be dissociated from the location of the file.

opoudjis added a commit to metanorma/isodoc that referenced this issue Sep 16, 2021
opoudjis added a commit to metanorma/metanorma that referenced this issue Sep 16, 2021
opoudjis added a commit to metanorma/metanorma.org that referenced this issue Sep 16, 2021
@opoudjis
Copy link
Contributor

opoudjis commented Sep 16, 2021

So, the base asset path is:

:base-asset-path: if given as an AsciiDoc document attribute

isodoc_options[:baseassetpath] in IsoDoc::XXX::HtmlConvert.new(isodoc_options), if directly calling Isodoc.

@opoudjis
Copy link
Contributor

@abunashir Back to you

opoudjis added a commit to metanorma/metanorma-standoc that referenced this issue Sep 16, 2021
@opoudjis opoudjis self-assigned this Sep 16, 2021
opoudjis added a commit to metanorma/metanorma that referenced this issue Sep 16, 2021
opoudjis added a commit to metanorma/metanorma-standoc that referenced this issue Sep 16, 2021
@abunashir
Copy link
Member

abunashir commented Sep 25, 2021

@ronaldtse: Let me clarify if I get that right - so, we basically want to add an option for baseassetpath to the site generator command and if user pass that option then we will need to pass it down to the metnorma compile interface?

cc: @opoudjis

@ronaldtse
Copy link
Contributor Author

@abunashir not quite but close! Specifically,

  1. when the site generator calls Metanorma to create the XML, it needs to pass the extra option of :base-asset-path: where it is set to the location of the adoc file. This enables Metanorma XML to contain the base asset path of it.

  2. when the site generator converts the XML into resulting formats, the Metanorma processor can read base-asset-path from the XML and know where to find the assets.

@abunashir
Copy link
Member

abunashir commented Oct 2, 2021

@ronaldtse: The sitemap generator is passing the source file (adoc) as it is without reading it or doing any changes. Unless we want to add custom behaviour for this options, wouldn't it be better to add this functionality to figure out the default base path from metanorma gem side?

Update:

I was trying to test it out with both options, but now looks like the word converter is throwing the same error now. I will try do dig deep and will keep you guys posted and widely enough its looking into the site directory 🤔

Screenshot 2021-10-02 at 17 28 38

@ronaldtse
Copy link
Contributor Author

Unless we want to add custom behaviour for this options, wouldn't it be better to add this functionality to figure out the default base path from metanorma gem side?

When Metanorma is used on its own for one document, i.e.

metanorma ./foo.adoc

All external paths are performed using the base relative path from ./.

In site generation, it looks like the source files get first moved into site/documents/ so the external assets can't be found?

@abunashir
Copy link
Member

The xml/html or other issues seems to be fixed but the word file still has the missing image issues - I will do more debugging later to see what exactly is happening there!

In site generation, it looks like the source files get first moved into site/documents/ so the external assets can't be found?

ronaldtse pushed a commit to metanorma/metanorma.org that referenced this issue Oct 5, 2021
opoudjis added a commit to metanorma/isodoc that referenced this issue Oct 12, 2021
opoudjis added a commit to metanorma/html2doc that referenced this issue Oct 12, 2021
opoudjis added a commit to metanorma/html2doc that referenced this issue Oct 12, 2021
@opoudjis
Copy link
Contributor

Word operation now fixed.

@opoudjis
Copy link
Contributor

Ronald has agreed to make dataimageuri opt-out rather than opt-in, so it will need to be given an explicit false argument.

opoudjis added a commit to metanorma/isodoc that referenced this issue Oct 12, 2021
@ronaldtse
Copy link
Contributor Author

So this particular task needs to be updated to say that when "data-image-uri" is false, then this issue applies.

opoudjis added a commit to metanorma/metanorma.org that referenced this issue Oct 12, 2021
opoudjis added a commit to metanorma/metanorma-standoc that referenced this issue Oct 12, 2021
opoudjis added a commit to metanorma/metanorma that referenced this issue Oct 12, 2021
@opoudjis
Copy link
Contributor

My work's complete. @abunashir Once you do your merge, I think this ticket is done.

ronaldtse pushed a commit to metanorma/metanorma.org that referenced this issue Oct 13, 2021
ronaldtse pushed a commit to metanorma/metanorma.org that referenced this issue Oct 13, 2021
abunashir added a commit that referenced this issue Oct 14, 2021
@opoudjis
Copy link
Contributor

Fixed rspec, but are you testing what is done with baseassetpath, @abunashir?

@abunashir
Copy link
Member

Thanks for fixing the spec, but I'm still having issue to generate word file, I've added the details to that PR. Regarding - the tests - we are only testing the delegation behaviour to that interface, and trust the metanorma gem to behave accordingly and also to avoid duplicate tests on both libraries.

@abunashir
Copy link
Member

Update: Since, @opoudjis set it as default value, so I assume we don't necessarily need to set the base_path anymore, can we close this one @ronaldtse, or would you still prefer to add it?

@ronaldtse
Copy link
Contributor Author

@abunashir in the case where the value is explicitly not set to the default, we will still have this problem... So we still need to fix it.

@ronaldtse ronaldtse moved this to Triage in Metanorma Nov 14, 2021
@ronaldtse ronaldtse moved this from Triage to Pending release in Metanorma Nov 14, 2021
@abunashir
Copy link
Member

Thanks @ronaldtse, with the recent merge on the PR, should it be resolved? can we close this issue?

@ronaldtse
Copy link
Contributor Author

Confirm fixed in metanorma/iso-10303-11 with :data-uri-image: false

image

Repository owner moved this from Pending release to Done in Metanorma Nov 30, 2021
@ronaldtse ronaldtse removed this from Metanorma Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants