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

Use same destination-dir as asciidoctor #39

Closed
Hocdoc opened this Issue Aug 14, 2014 · 21 comments

Comments

Projects
None yet
4 participants
@Hocdoc

Hocdoc commented Aug 14, 2014

I compile my documents from the CLI like this:

asciidoctor -r  asciidoctor-diagram   -D target/   source/samle.adoc

asciidoctor will create the output sample.html in the target/ directory.
But asciidoctor-diagram creates all the PNG files in the active directory . and not in target/. Therefore, the image links in sample.html are also wrong.

@pepijnve

This comment has been minimized.

Show comment
Hide comment
@pepijnve

pepijnve Aug 14, 2014

Contributor

@mojavelinux what is the correct way to get to this value? The code is currently already trying to use the outdir attribute, but afaict that gets set too late in the process (asciidoctor.rb:1465). I can't seem to find any other way to access the output path.

Contributor

pepijnve commented Aug 14, 2014

@mojavelinux what is the correct way to get to this value? The code is currently already trying to use the outdir attribute, but afaict that gets set too late in the process (asciidoctor.rb:1465). I can't seem to find any other way to access the output path.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 14, 2014

Member

This an issue I've hit building extensions too and definitely something that core needs to provide access to via a public API. I'll look at my notes and document the workaround we need to use for now.

Member

mojavelinux commented Aug 14, 2014

This an issue I've hit building extensions too and definitely something that core needs to provide access to via a public API. I'll look at my notes and document the workaround we need to use for now.

@pepijnve

This comment has been minimized.

Show comment
Hide comment
@pepijnve

pepijnve Aug 16, 2014

Contributor

I had a look at the mathoid extension for inspiration. At https://github.com/asciidoctor/asciidoctor-extensions-lab/blob/master/lib/mathoid-treeprocessor/extension.rb#L25 it's determining the target path using AbstractNode#normalize_system_path. I'm changing the diagram code to use the same method instead of the custom resolution code I previously had. AFAICT, this doesn't take the target directory into account though.

Contributor

pepijnve commented Aug 16, 2014

I had a look at the mathoid extension for inspiration. At https://github.com/asciidoctor/asciidoctor-extensions-lab/blob/master/lib/mathoid-treeprocessor/extension.rb#L25 it's determining the target path using AbstractNode#normalize_system_path. I'm changing the diagram code to use the same method instead of the custom resolution code I previously had. AFAICT, this doesn't take the target directory into account though.

@maxandersen

This comment has been minimized.

Show comment
Hide comment
@maxandersen

maxandersen Aug 17, 2014

This also affects use of asciidoctor-diagram in awestruct and there is another funky caveat.

Not only are the images generated in the current directory instead of the dir that the asciidoctor files are being rendered too it also generates urls with double /.

i.e. I'm getting .//diag-8ca7aac502681b731543ec2ec69016ed.png (notice the //) instead of expected just diag-8ca7aac502681b731543ec2ec69016ed.png

Note, the latter might be an awestruct specific quirk but just wanted to mention it since everything else we got (image links etc.) generates the proper relative url.

p.s. is there a way to control this generate diagram name instead of being what look like sha based ? (I would like to have stable links to these images that does not get affected by changes in the document)

maxandersen commented Aug 17, 2014

This also affects use of asciidoctor-diagram in awestruct and there is another funky caveat.

Not only are the images generated in the current directory instead of the dir that the asciidoctor files are being rendered too it also generates urls with double /.

i.e. I'm getting .//diag-8ca7aac502681b731543ec2ec69016ed.png (notice the //) instead of expected just diag-8ca7aac502681b731543ec2ec69016ed.png

Note, the latter might be an awestruct specific quirk but just wanted to mention it since everything else we got (image links etc.) generates the proper relative url.

p.s. is there a way to control this generate diagram name instead of being what look like sha based ? (I would like to have stable links to these images that does not get affected by changes in the document)

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 17, 2014

Member

Nope. Due to legacy logic inherited from AsciiDoc Py, Asciidoctor only takes the target directory into account when writing the main output file. In fact, that directory isn't available to any extension point except the Writer.

...having said that, there's no reason we can't expose this information earlier. In fact, it should be know as soon as we finish parsing the document header. We just need to file an issue request into core and we can make this available as soon as the next release.

There's still a larger issue to discuss about file resolution in general, but that discussion doesn't have to be a blocker to get what we need to the extensions.

Member

mojavelinux commented Aug 17, 2014

Nope. Due to legacy logic inherited from AsciiDoc Py, Asciidoctor only takes the target directory into account when writing the main output file. In fact, that directory isn't available to any extension point except the Writer.

...having said that, there's no reason we can't expose this information earlier. In fact, it should be know as soon as we finish parsing the document header. We just need to file an issue request into core and we can make this available as soon as the next release.

There's still a larger issue to discuss about file resolution in general, but that discussion doesn't have to be a blocker to get what we need to the extensions.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 17, 2014

Member

One way to workaround this problem in the shortrun is to introduce a new attribute, imagesoutdir. We'll likley end up doing something like this in core anyway, so you could support it now and then migration will just happen.

Member

mojavelinux commented Aug 17, 2014

One way to workaround this problem in the shortrun is to introduce a new attribute, imagesoutdir. We'll likley end up doing something like this in core anyway, so you could support it now and then migration will just happen.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 17, 2014

Member

The biggest design flaw with assets in AsciiDoc Python is that it didn't separate input and output attributes. As you can probably imagine, imagesdir doesn't tell you whether that is for reading or writing files. Thus far, we sort of use it for both with some rudimentary intelligence. However, there are plenty of situations in which they are going to be completely orthogonal.

Member

mojavelinux commented Aug 17, 2014

The biggest design flaw with assets in AsciiDoc Python is that it didn't separate input and output attributes. As you can probably imagine, imagesdir doesn't tell you whether that is for reading or writing files. Thus far, we sort of use it for both with some rudimentary intelligence. However, there are plenty of situations in which they are going to be completely orthogonal.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 17, 2014

Member

Keep in mind that an extension, like Asciidoctor Diagram, can look for any attribute name it wants. We don't want to go crazy, but it's a schemaless design...so no reason to feel constrained by what is there.

Member

mojavelinux commented Aug 17, 2014

Keep in mind that an extension, like Asciidoctor Diagram, can look for any attribute name it wants. We don't want to go crazy, but it's a schemaless design...so no reason to feel constrained by what is there.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 17, 2014

Member

For the record, I think generating the images in the source directory when using Awestruct is a good thing. You should need to regenerate the images on every build...and Asciidoctor Diagram will intelligently write a new image when necessary...which you then check-in to source control. It's a perfectly fine model in my mind.

This is still an issue. I just wouldn't let it become a blocker for adoption.

Member

mojavelinux commented Aug 17, 2014

For the record, I think generating the images in the source directory when using Awestruct is a good thing. You should need to regenerate the images on every build...and Asciidoctor Diagram will intelligently write a new image when necessary...which you then check-in to source control. It's a perfectly fine model in my mind.

This is still an issue. I just wouldn't let it become a blocker for adoption.

@maxandersen

This comment has been minimized.

Show comment
Hide comment
@maxandersen

maxandersen Aug 17, 2014

I must say I prefer highly that I don't have to be forced to include more binary images into the git repos - so as long as I can do a .gitignore on them that would be ok.

maxandersen commented Aug 17, 2014

I must say I prefer highly that I don't have to be forced to include more binary images into the git repos - so as long as I can do a .gitignore on them that would be ok.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 18, 2014

Member

As I think about this more, the combination of outdir plus imagesdir should be the fallback, "it just works" behavior (once outdir is made available). The imagesoutdir would be checked first, but considered a specialized use case.

Member

mojavelinux commented Aug 18, 2014

As I think about this more, the combination of outdir plus imagesdir should be the fallback, "it just works" behavior (once outdir is made available). The imagesoutdir would be checked first, but considered a specialized use case.

@maxandersen

This comment has been minimized.

Show comment
Hide comment
@maxandersen

maxandersen Aug 19, 2014

Just to grok the status - for now what I'm seeing is that I cannot use asciidoctor-diagram with awestruct, there is no way to tweak/convince asciidoctor-diagram to put its files in a proper place, correct ?

All of the above is suggestions on future release behavior, right?

maxandersen commented Aug 19, 2014

Just to grok the status - for now what I'm seeing is that I cannot use asciidoctor-diagram with awestruct, there is no way to tweak/convince asciidoctor-diagram to put its files in a proper place, correct ?

All of the above is suggestions on future release behavior, right?

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 19, 2014

Member

Of course, all things are possible when you have extensions :) All you need to do is add a script that runs after the files are processed and move the files from the source to the destination. That script could either be an Asciidoctor postprocessor or something that runs at the end of the Awestruct pipeline.

It's just not very convenient right now :)

Member

mojavelinux commented Aug 19, 2014

Of course, all things are possible when you have extensions :) All you need to do is add a script that runs after the files are processed and move the files from the source to the destination. That script could either be an Asciidoctor postprocessor or something that runs at the end of the Awestruct pipeline.

It's just not very convenient right now :)

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 19, 2014

Member

I think a first step in Asciidoctor Diagram is to honor the 'imagesoutdir', which we can set to the _site directory in Awestruct. Then, we'll go back and fix the default behavior to honor the outdir once I can get that info to the extension.

To summarize, the workaround options, in order of when you can apply them are:

  1. Custom Asciidoctor postprocessor to move the files manually
  2. Asciidoctor Diagram honoring an 'imagesoutdir' attribute
  3. Asciidoctor Diagram uses the 'outdir' to locate the target images folder, once that attribute becomes available
Member

mojavelinux commented Aug 19, 2014

I think a first step in Asciidoctor Diagram is to honor the 'imagesoutdir', which we can set to the _site directory in Awestruct. Then, we'll go back and fix the default behavior to honor the outdir once I can get that info to the extension.

To summarize, the workaround options, in order of when you can apply them are:

  1. Custom Asciidoctor postprocessor to move the files manually
  2. Asciidoctor Diagram honoring an 'imagesoutdir' attribute
  3. Asciidoctor Diagram uses the 'outdir' to locate the target images folder, once that attribute becomes available
@pepijnve

This comment has been minimized.

Show comment
Hide comment
@pepijnve

pepijnve Aug 19, 2014

Contributor

3 is what the code currently does (see https://github.com/asciidoctor/asciidoctor-diagram/blob/master/lib/asciidoctor-diagram/util/diagram.rb#L117), but that depends on asciidoctor exposing the outdir attribute.

I will add 2 as soon as possible. @mojavelinux supposethe target directory is set to out and imagesdir is set to images. What should imagesoutdir be set to then? Would you expect images to be written to <imagesoutdir>/<imagesdir> or to <imagesoutdir>? From your explanation above I think you mean the the first option, but I want to be sure. So you get for instance
imagesoutdir: _site
imagesdir: images
and asciidoctor-diagram ends up writing the images to _site/images. In other words, imagesoutdir is identical to outdir and just serves as a stop-gap solution until the latter becomes available. Is that correct?

Contributor

pepijnve commented Aug 19, 2014

3 is what the code currently does (see https://github.com/asciidoctor/asciidoctor-diagram/blob/master/lib/asciidoctor-diagram/util/diagram.rb#L117), but that depends on asciidoctor exposing the outdir attribute.

I will add 2 as soon as possible. @mojavelinux supposethe target directory is set to out and imagesdir is set to images. What should imagesoutdir be set to then? Would you expect images to be written to <imagesoutdir>/<imagesdir> or to <imagesoutdir>? From your explanation above I think you mean the the first option, but I want to be sure. So you get for instance
imagesoutdir: _site
imagesdir: images
and asciidoctor-diagram ends up writing the images to _site/images. In other words, imagesoutdir is identical to outdir and just serves as a stop-gap solution until the latter becomes available. Is that correct?

@pepijnve pepijnve added this to the 1.2.1 milestone Aug 19, 2014

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 20, 2014

Member

I want to point out this this is also an issue in the Maven and Gradle plugins (and in the doclet for Java). More and more we need to be able to fine tune where the images end up, because we don't always have control directly to help them to the right location. (Of course, there is always the extension route, but obviously, that's not the ideal scenario).

When you have a pull request ready, I'll be happy to review and test. Once we have the Maven, Gradle and doclet plugins released with the ability to load Asciidoctor Diagram, we'll have more places to test our solution.

Member

mojavelinux commented Aug 20, 2014

I want to point out this this is also an issue in the Maven and Gradle plugins (and in the doclet for Java). More and more we need to be able to fine tune where the images end up, because we don't always have control directly to help them to the right location. (Of course, there is always the extension route, but obviously, that's not the ideal scenario).

When you have a pull request ready, I'll be happy to review and test. Once we have the Maven, Gradle and doclet plugins released with the ability to load Asciidoctor Diagram, we'll have more places to test our solution.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 20, 2014

Member

I've got Asciidoctor Diagram working in the Gradle plugin, Maven plugin and Asciidoclet, so we're going to have a lot of places we can test it very soon!

Member

mojavelinux commented Aug 20, 2014

I've got Asciidoctor Diagram working in the Gradle plugin, Maven plugin and Asciidoclet, so we're going to have a lot of places we can test it very soon!

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 21, 2014

Member

You can now try this with the Gradle plugin example project:

https://github.com/mojavelinux/asciidoctor-gradle-example

Member

mojavelinux commented Aug 21, 2014

You can now try this with the Gradle plugin example project:

https://github.com/mojavelinux/asciidoctor-gradle-example

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Aug 26, 2014

Member

I haven't forgotten about this one. Just been super busy.

Member

mojavelinux commented Aug 26, 2014

I haven't forgotten about this one. Just been super busy.

@pepijnve pepijnve modified the milestones: 1.2.1, 1.3.0 Aug 28, 2014

@pepijnve

This comment has been minimized.

Show comment
Hide comment
@pepijnve

pepijnve Aug 28, 2014

Contributor

#44 should cover this. I forgot to make feature/issue branches so there's a bunch of other stuff in that PR as well I'm afraid.

Contributor

pepijnve commented Aug 28, 2014

#44 should cover this. I forgot to make feature/issue branches so there's a bunch of other stuff in that PR as well I'm afraid.

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Nov 26, 2014

Member

I've done a lot of thinking about this problem and posted my conclusions in the following thread: asciidoctor/asciidoctor#1203 (comment)

What it comes down to is this...extensions can't know the destination dir since it's not yet resolved at the time the extensions are loaded and executed (with the exception of postprocessors).

The extensions can know the intended target directory, as passed to the API. I've made that value (after path expansion) available via document.options[:to_dir]. It's possible that it will differ from outdir if it violates the jail in server or secure mode, but it's almost guaranteed to match in unsafe and safe modes.

With that said, I still recommend that build plugins (like the Maven and Gradle plugins) set the outdir attribute explicitly, which Asciidoctor Diagram can then rely on. I like the idea of having imagesoutdir as an alternative way of passing this directory...and to allow it to be different from outdir.

I think that if outdir and imagesoutdir are not set, Asciidoctor Diagram should fall back to document.options[:to_dir] if Asciidoctor::VERSION >= 1.5.2 and the value is specified. Otherwise, I think this issue can be closed with a recommendation to set the imagesoutdir in the build configuration as a best practice.

Member

mojavelinux commented Nov 26, 2014

I've done a lot of thinking about this problem and posted my conclusions in the following thread: asciidoctor/asciidoctor#1203 (comment)

What it comes down to is this...extensions can't know the destination dir since it's not yet resolved at the time the extensions are loaded and executed (with the exception of postprocessors).

The extensions can know the intended target directory, as passed to the API. I've made that value (after path expansion) available via document.options[:to_dir]. It's possible that it will differ from outdir if it violates the jail in server or secure mode, but it's almost guaranteed to match in unsafe and safe modes.

With that said, I still recommend that build plugins (like the Maven and Gradle plugins) set the outdir attribute explicitly, which Asciidoctor Diagram can then rely on. I like the idea of having imagesoutdir as an alternative way of passing this directory...and to allow it to be different from outdir.

I think that if outdir and imagesoutdir are not set, Asciidoctor Diagram should fall back to document.options[:to_dir] if Asciidoctor::VERSION >= 1.5.2 and the value is specified. Otherwise, I think this issue can be closed with a recommendation to set the imagesoutdir in the build configuration as a best practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment