asciidoctor / atom-asciidoc-preview

βš› AsciiDoc preview for the Atom editor.
https://atom.io/packages/asciidoc-preview
MIT License
141 stars 42 forks source link

Include macro not working using an attribute in the path #189

Closed mattadamson closed 7 years ago

mattadamson commented 8 years ago

Hi

We use the include macro however we also specify the path using an ascii doctor attribute eg

include::{generatedclient}/operations/postContactCreate.adoc[]

After adding the attribute generatedclient into the default attributes settings we receive a message in the preview pane that the directive can't be resolved.

Unresolved directive in <stdin> - include::D:/data/git/myapp/target/docs/operations/postContactCreate.adoc[]

The file obviously exists in that path and it appears the attribute is being found and expanded here.

Appreciate your thoughts? I also have changed to unsafe mode

ldez commented 8 years ago

Hi,

If you are using absolute path, you must change your settings to accept absolute path.

Go on the package settings, change "safe mode" and base directory option.

mojavelinux commented 8 years ago

As @ldez suggested, this is not about the attribute reference. It's about the value of the attribute.

mattadamson commented 8 years ago

thanks however what do you mean value of the attribute, value of which attribute? In the configuration for base directory I tried setting the value to "-" which indicates using an absolute path. If I enter a base directory this also fails.

I'll try clarify some more details of the full set up consider we have this as the base root directory for adoc files which also includes the main.adoc linking to other nested adoc files

D:\swagger-doc-generator\src\docs\asciidoc

If we edit a main.adoc file directly in Atom from that folder links such as

include::loginoverview.adoc[]
include::actionoverview.adoc[]

Indicate

Unresolved directive in <stdin> - include::loginoverview.adoc[]

We also have some includes with attribute references such as

include::{generatedclient}/overview.adoc[]

however these resolve fine when generatedclient is included as an attribute in the settings attributes section with an absolute path to the folder i.e. generatedclient=D:/swagger-doc-generator/target/asciidoc

Thanks

ldez commented 8 years ago

I suggest you to use relative path instead of absolute path if you can.

mattadamson commented 8 years ago

thanks @ldez , we had two concepts of the base ascii directory and a generated folder as we were using other generated adoc files from another module swagger2markup. I can see if we can refactor our links to try and make them relative although it will be more complex.

ldez commented 8 years ago

Can you provide your files trees (both cases) and more detailed samples ?

mojavelinux commented 8 years ago

what do you mean value of the attribute, value of which attribute?

What I'm saying is that the problem is not the attribute, it's the include path, as @ldez has suggested.

This does raise the question about whether absolute paths should be supported. I think they should as long as they resolve to a path within the project and/or base_dir. In other words, we should be able to convert the absolute path to a relative path automatically (speaking in theory). I could understand if the absolute path was somewhere outside the project, but in this case it is not.

mojavelinux commented 8 years ago

In other words, what I'm saying is that this has nothing to do with attributes and everything to do with the include path itself. The include directive never sees the attribute reference because it's already resolved by the time the include directive is processed.

mattadamson commented 8 years ago

thanks @mojavelinux , from

I could understand if the absolute path was somewhere outside the project, but in this case it is not.

Actually the generatedclient attribute is a folder that exists outside the main ascii doc root folder as in the first message. Are you suggesting this could be the reason why it fails? If it's an absolute path why couldn't it work though and why should it need to exist as a sub folder?

Thanks

mojavelinux commented 8 years ago

Are you suggesting this could be the reason why it fails? If it's an absolute path why couldn't it work though and why should it need to exist as a sub folder?

That would depend on the safe mode. If the safe mode is unsafe, then it is at least permitted to work.

This really comes down to what the file IO adapters permit. Asciidoctor.js has several because there are no two JavaScript environments that are the same. When running in Node.js (as Atom does), it should be possible to resolve absolute paths. But that's where we need to be focused.

ldez commented 8 years ago

@mattadamson any news?

ldez commented 8 years ago

@mattadamson any news?

mattadamson commented 8 years ago

Hi @mojavelinux @ldez I went back to review this again and simply tried using a full path reference to an adoc file which also failed e.g.

include::D:\swagger-doc-generator\src\docs\asciidoc\myfile.adoc[]

which produces the "Unresolved directive in " type error

I have unsafe selected so can't understand why it's not working. Is there a debug level of logging we could enable perhaps?

mojavelinux commented 8 years ago

I'm not sure if it changes things, but I recommend using forward slashes instead of backslashes. Windows recognizes both, but the forward slashes cause less problems.

...having said that, I'm not sure if absolute paths actually work. I can't recall one way or the other.

mattadamson commented 8 years ago

thanks @mojavelinux I actually tried forward slashes too. Absolute paths do work fine when generating the documentation so wonder what's different in the atom extension.

mattadamson commented 8 years ago

Also could this be related to the configuration value "Base directory" in the atom package settings. The help indicates set to - to use an absolute path. Does this mean it should be set to simply "-" in this case? Or should I specify the root folder using an absolute path and then use the other attributes we set to ascii doctor as relative paths from this. Would this work if I open any adoc file in the editor e.g. if in the base directory or an adoc file which is two sub folders down?

ldez commented 8 years ago

If you define a existing "base directory", you must define attributes related to this path (eg relative to the root base directory)

mojavelinux commented 8 years ago

Generally speaking, you almost never want to use a base directory. It's more of a security feature and it happens to have side effects to portability at the moment. It should be unset (however that is).

I agree the message about the special value "-" could be clearer. It's not obvious what is meant by "an absolute path".

mattadamson commented 8 years ago

Hi

I'd really like to resolve this and fix the "Unresolved directive in " errors. I'm not clear we understand the root cause yet. Is it possible I could even debug locally to help? If so how would I do this?

Many thanks

ldez commented 8 years ago

It's a problem related to AsciiDoctor.js, not with the preview.

Can you provide a files trees (both cases) and more detailed samples ?

andmos commented 8 years ago

I guess you guys know this, but the same error accures if the .adoc file set to be included is located on a network share (Windows client that is):

include::\\WindowsShare\Folder\Templates\Asciidoc\styles\my-core-styles.adoc[]

Preview:

Unresolved directive in <stdin> -include::\\WindowsShare\Folder\Templates\Asciidoc\styles\my-core-styles.adoc[]
mattadamson commented 8 years ago

@ldez thanks. It's a large project and don't want it publicly visible on this forum post. Can I send it directly to you at all? Also I'd love to get this resolved can I debug the ascii doc preview extension through atom easily? If there is a page / instructions I can also try.

ldez commented 8 years ago

If you don't want to provide your files, you can build a quick sample with fake content.

We need this to analyze your problem or to validate a solution.

But I repeat : it's not a bug with preview package but a behavior of AsciiDoctor.js.

mattadamson commented 8 years ago

thanks @ldez, I've managed to get a nice simple adoc set of files which include in a few folders to show the issue.
includeDirectiveIssueExample.zip

I set the atom ascii doc attributes to the following

baseascidoc=/Users/madamson/testadoc pathclient=/Users/madamson/testadoc/client platform=opal platform-opal env=browser env-browser source-highlighter=highlight.js data-url!

So when you open main.adoc and preview you see

[_testb] Unresolved directive in /Users/madamson/testadoc/subpage.adoc - include::/Users/madamson/testadoc/subfolder/testA.adoc[] Unresolved directive in <stdin> - include::/Users/madamson/testadoc/client/testB.adoc[]

Assuming the zip is extracted into the folder /Users/madamson/testadoc you can see those adoc files and folders do exist in that location. I'd be very interested to know why this occurs and if / how we can fix it. Let me know if you want to help / debug too with instructions

Many thanks

mattadamson commented 8 years ago

Does anyone know how to remove the "more-information-needed" label as I can't see in the GUI?

ldez commented 8 years ago

Only maintainers can do that πŸ˜‰

ldez commented 8 years ago

How to solve:

  1. Change AsciiDoc Preview options:
    • Safe Mode: unsafe
    • Base directory: /Users/madamson/testadoc
    • Default attributes: platform=opal platform-opal env=browser env-browser source-highlighter=highlight.js data-uri! pathclient=/client
  2. Change subpage.adoc content:
include::{baseascidoc}/subfolder/testA.adoc[]

to

include::/subfolder/testA.adoc[]

or If you really need to use an attribute baseascidoc, add baseascidoc=. to AsciiDoc Preview option "Default attributes".


Result:

capture du 2016-10-22 14-44-01

ldez commented 8 years ago

Remind you cannot mix relative and absolute file, you must choose one of them.

ldez commented 8 years ago

I created a more complex sample (I deliberately chose to use exclusively absolute paths):

Project structure:

.
β”œβ”€β”€ mainfolder
β”‚   β”œβ”€β”€ main.adoc
β”‚   └── subpage.adoc
└── subfolder
    └── pageA.adoc
Main Page

include::{subfolderPath}/pageA.adoc[]

include::{baseascidoc}/subpage.adoc[]
Sub Page

include::{subfolderPath}/pageA.adoc[]
I'm the pageA.

Asciidoc Preview options:

And I detected a problem: pageA.adoc is correctly resolved in main.adoc but fail when the include is in another include (eg. subpage.adoc)

WARNING:
/home/myuser/issue-189-plus/mainfolder/subpage.adoc:
line 3:
include file not readable:
/home/myuser/issue-189-plus/mainfolder//home/myuser/issue-189-plus/subfolder/pageA.adoc

@mojavelinux @Mogztter I think it's a bug with Asciidoctor.js : base_dir attribute is not propagate when the value is -.

ggrossetie commented 8 years ago

Indeed I can reproduce this error :disappointed:

ggrossetie commented 8 years ago

I think I found the root cause \o/

We are using a special case for Opal in the preprocess_include method in Asciidoctor core: https://github.com/asciidoctor/asciidoctor/blob/b3ff32e27ae88abbf6178f3b85543ccbd2f07c10/lib/asciidoctor/reader.rb#L838-L847

This special case is unnecessary (or at least not necessary anymore on Node) so I think we should check that the platform is browser: "TODO only use this logic if env-browser is set".

mattadamson commented 8 years ago

@ldez @Mogztter this sounds very promising thanks. Is it possible I can even try a code change here to see if it works on my local environment or this example or have you done that?

ggrossetie commented 8 years ago

You could remove the special case in the opal.js file but that's a dirty hack.

To fix this issue faster, I think I can monkey patch the reader.rb file in Asciidoctor.js and release a new version but I first want to hear from @mojavelinux (in case I missed something)

mattadamson commented 8 years ago

thanks @Mogztter I'd like to try that though to validate this also works for my larger project outside this example. I'm a novice with atom and the ascii doctor package. So how would I change opal.js on my local installation? Is this js file part of the atom installed package as a file can simply open, edit and save?

ggrossetie commented 8 years ago

Hello @mattadamson

So how would I change opal.js on my local installation? Is this js file part of the atom installed package as a file can simply open, edit and save?

I believe so. I do not use Atom much so I do not know where you can find opal.js but you get the idea.

Once you've found this file I can give the lines you will need to modify.

This is a temporary fix until a new version of Asciidoctor.js is released and a new version of this plugin is released using this new version of Asciidoctor.js

ggrossetie commented 8 years ago

@mattadamson If you are using Linux, the path should be .atom/packages/asciidoc-preview/node_modules/asciidoctor.js/node_modules/opal-npm-wrapper/index.js

mattadamson commented 8 years ago

thanks @Mogztter I found the opal-npm-wrapper/index.js file however I can't see any logic similar to the reader.rb code you mentioned with the TODO and env-browser reference. What should I change here to try this fix?

ggrossetie commented 8 years ago

Great! It's a bit tricky since you will have to modify the Asciidoctor.js module:

  1. Open ~/.atom/packages/asciidoc-preview/node_modules/asciidoctor.js/package.json
  2. Replace this line "main": "dist/npm/asciidoctor-core.min.js" by "main": "dist/npm/asciidoctor-core.js"
  3. Edit ~/.atom/packages/asciidoc-preview/node_modules/asciidoctor.js/dist/npm/asciidoctor-core.js
  4. Search for the text "maximum include depth"
  5. The code should looks like:
        // ...
        } else if ((($a = (($b = $rb_gt((abs_maxdepth = self.maxdepth['$[]']("abs")), 0)) ? $rb_ge(self.include_stack.$size(), abs_maxdepth) : $rb_gt((abs_maxdepth = self.maxdepth['$[]']("abs")), 0))) !== nil && (!$a.$$is_boolean || $a == true))) {
          self.$warn("asciidoctor: ERROR: " + (self.$line_info()) + ": maximum include depth of " + (self.maxdepth['$[]']("rel")) + " exceeded");
          return false;
        } else if ($rb_gt(abs_maxdepth, 0)) {
          if ((($a = Opal.get('RUBY_ENGINE_OPAL')) !== nil && (!$a.$$is_boolean || $a == true))) {
            target_type = "file";
            include_file = path = (function() {if ((($a = self.include_stack['$empty?']()) !== nil && (!$a.$$is_boolean || $a == true))) {
              if (Opal.get('Dir').$pwd()['$=='](self.document.$base_dir())) {
                return target
                } else {
                return (Opal.get('File').$join(self.dir, target))
              }
              } else {
              return Opal.get('File').$join(self.dir, target)
            }; return nil; })();
          } else if ((($a = $scope.get('Helpers')['$uriish?'](target)) !== nil && (!$a.$$is_boolean || $a == true))) {
            if ((($a = self.document.$attributes()['$has_key?']("allow-uri-read")) !== nil && (!$a.$$is_boolean || $a == true))) {
              } else {
              self.$replace_next_line("link:" + (target) + "[]");
              return true;
            };
            target_type = "uri";
            include_file = path = target;
            // ...

Edit the code to remove the if condition about RUBY_ENGINE_OPAL:

        // ...
        } else if ((($a = (($b = $rb_gt((abs_maxdepth = self.maxdepth['$[]']("abs")), 0)) ? $rb_ge(self.include_stack.$size(), abs_maxdepth) : $rb_gt((abs_maxdepth = self.maxdepth['$[]']("abs")), 0))) !== nil && (!$a.$$is_boolean || $a == true))) {
          self.$warn("asciidoctor: ERROR: " + (self.$line_info()) + ": maximum include depth of " + (self.maxdepth['$[]']("rel")) + " exceeded");
          return false;
        } else if ($rb_gt(abs_maxdepth, 0)) {
          if ((($a = $scope.get('Helpers')['$uriish?'](target)) !== nil && (!$a.$$is_boolean || $a == true))) {
            if ((($a = self.document.$attributes()['$has_key?']("allow-uri-read")) !== nil && (!$a.$$is_boolean || $a == true))) {
              } else {
              self.$replace_next_line("link:" + (target) + "[]");
              return true;
            };
            target_type = "uri";
            include_file = path = target;
            // ...

NOTE: The else if condition became an if condition:

Please tell me if this is working for you :smile:

mattadamson commented 8 years ago

thanks @Mogztter for the detailed instructions. It works perfectly now in the example and also my larger documentation project.

mattadamson commented 8 years ago

I presume everytime we update the package through atom those JS changes will simply be reverted. I'm going to simply attach a copy of the JS file modified for our team to overlay in the meantime in their local environments to use this fix until it's resolved in the main package.

ggrossetie commented 8 years ago

Excellent!

Yes I think you are right. Hopefully a new version with this fix will be available soon ;)

Le 31 oct. 2016 11:31 AM, "mattadamson" notifications@github.com a Γ©crit :

I presume everytime we update the package through atom those JS changes will simply be reverted. I'm going to simply attach a copy of the JS file modified for our team to overlay in the meantime in their local environments to use this fix until it's resolved in the main package.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/asciidoctor/atom-asciidoc-preview/issues/189#issuecomment-257260708, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUV3NSTlYgu7f54GzK5QUsjlDudYRiVks5q5cOEgaJpZM4JBB-D .

mojavelinux commented 7 years ago

Let's get this fixed upstream. I'm starting to think about a 1.5.6 release, so let's prioritize that fix. @Mogztter is it this? https://github.com/asciidoctor/asciidoctor/pull/1898

ggrossetie commented 7 years ago

@mojavelinux yes :+1: