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

resolves #276 try to use KindleGen/EPUBCheck binary from $PATH #277

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

slonopotamus
Copy link
Contributor

No description provided.

@mojavelinux
Copy link
Member

You are correct to only do this if the KINDLEGEN env var is not set. That gives the user the ability to avoid the use of the kindlegen on the PATH, which is an important for security, I would think.

@slonopotamus slonopotamus force-pushed the kindlegen-path branch 4 times, most recently from efc2d04 to 88c1c63 Compare January 27, 2020 11:29
@slonopotamus slonopotamus changed the title resolves #276 try to use KindleGen binary from $PATH resolves #276 try to use KindleGen/EPUBCheck binary from $PATH Jan 27, 2020
end
argv = [::Kindlegen.command.to_s]
def get_kindlegen_command
unless (result = ENV['KINDLEGEN']).nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like that besides finding it in the environment there is another way of locating the binary i.e. via an attribute. This allows external downloaders to set the path. For instance in Gradle we are forced to run this out of process i.e. another JVM because of the environmental variable, but if there is a way of setting it, then we can utilise in-process generation.

Copy link
Contributor Author

@slonopotamus slonopotamus Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, via document attribute? That makes sense, doing things in-process (especially, on JRuby) is definitely preferrable.

Copy link
Contributor Author

@slonopotamus slonopotamus Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mojavelinux could suggest an attribute name for this so we don't acidentally clash with other asciidoctor machinery? Something like ebook-kindlegen-binary-path?

Copy link
Contributor Author

@slonopotamus slonopotamus Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ysb33r I think, the order of preference will be:

  1. env var
  2. attribute
  3. kindlegen gem
  4. PATH

Copy link
Contributor Author

@slonopotamus slonopotamus Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more thinking, I want to make attribute higher priority than env var. Case: user has env var defined and calls asciidoctor-epub3 -a ebook-kindlegen-binary-path=/path/to/kindlegen <book>. It is clear that their intent is to override global behavior for this particular invocation.

Copy link
Contributor Author

@slonopotamus slonopotamus Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mojavelinux are there any security concerns about passing kindlegen path through document attributes? Is there any alternative way to pass settings to Asciidoctor::Epub3::Packager that bypass the document itself?

I see that Converter::initialize accepts opts hash, maybe that's a better way?

Copy link
Contributor Author

@slonopotamus slonopotamus Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ysb33r I've updated PR with document attributes to specify KindleGen/EPUBCheck executable location, but still waiting for @mojavelinux to review this.

And it deserves to be documented, I think.

@slonopotamus slonopotamus force-pushed the kindlegen-path branch 3 times, most recently from a8b2983 to 9e981d2 Compare January 27, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants