Skip to content
This repository has been archived by the owner on Mar 18, 2022. It is now read-only.

Add new option to allow override pdflatex binary #18

Closed
wants to merge 2 commits into from
Closed

Add new option to allow override pdflatex binary #18

wants to merge 2 commits into from

Conversation

skirpichev
Copy link
Contributor

Any pdflatex-compatible binary could be used instead. For example, xelatex (as requested in readthedocs/readthedocs.org#1556).

This is a very primitive version of that can be used to solve mentioned issue. Some other variants are mentioned in the issue thread.

Any pdflatex-compatible binary could be used instead.  For example,
xelatex (as requested in readthedocs/readthedocs.org#1556).
@skirpichev
Copy link
Contributor Author

@agjohnson, any feedback?

@asr
Copy link

asr commented Jan 9, 2017

I'm interested in the fix of #1556. Any feedback on this PR?

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Sorry to leave this hanging for so long. Seems we could use some updates to the spec.rst file for this. This should make sense otherwise, with a companion patch to rtfd/readthedocs.org to use this setting.

@@ -291,6 +293,13 @@ def validate_conda(self):

self['conda'] = conda

def validate_pdflatex(self):
if 'pdflatex' not in self.raw_config:
self['pdflatex'] = validate_string('pdflatex')
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no need to validate this string

@agjohnson agjohnson added PR: work in progress Pull request is not ready for full review and removed Working labels Feb 9, 2017
@skirpichev
Copy link
Contributor Author

Ok, I did requested change.

This should make sense otherwise, with a companion patch to rtfd/readthedocs.org to use this setting.

Maybe it's better to use latex_engine option from conf.py? In this case - probably no changes should be made here, but all in rtfd/readthedocs.org repo.

@skirpichev
Copy link
Contributor Author

Test failure seems to be unrelated - master branch is broken.

@humitos
Copy link
Member

humitos commented Feb 26, 2017

@skirpichev I'm not the one taking the decisions but I would suggest to add documentation (maybe in spec.rst), and also changing the name of the pdflatex option to something more descriptive like pdf_bin or pdf_binary. Although, I think it shouldn't be a simple string but a multiple choice value because if it will be executed as is, RTD would be executing arbitrary commands.

On the other hand, this PR will need another one into the readthedocs.org code to finally execute this command, right?

@skirpichev
Copy link
Contributor Author

but I would suggest to add documentation

Sure. But I don't know where it should be. Most yml settings are documented now in readthedocs.org main repo.

something more descriptive like pdf_bin or pdf_binary

Maybe. But your suggestions looks even more cryptic.

I think it shouldn't be a simple string but a multiple choice value because if it will be executed as is, RTD would be executing arbitrary commands.

Or just add more validation for this option, e.g. test if this value resolves to some binary in $PATH.
But I think, multivalued option is fine. Thank you.

On the other hand, this PR will need another one into the readthedocs.org code to finally execute this command, right?

Sure. I'm waiting here, because otherwise PR against readthedocs.org doesn't make sense.

Here is another way to solve issue. So, it's possible without changes in readthedocs-build, in principle.

@agjohnson
Copy link
Contributor

So, this seems a little more complicated than just swapping out the binary used. I agree with @humitos that the option shouldn't be just a string, and that it should probably be moved to a different namespace in the space.

Bear with us, this is going to be a spec related decision.

I believe we'll be adding a number of options to configure sphinx eventually, so perhaps putting it under the sphinx namespace makes more sense. Here are some options:

sphinx:
  latex:
    binary: (?:xelatex|latex)
sphinx:
  latex_binary: (?:xelatex|latex)

I think i lean towards option 1, perhaps there are others? This would take adding parsing of the sphinx and sphinx['latex'] dictionaries, and then finally the binary parameter.

@skirpichev
Copy link
Contributor Author

I believe we'll be adding a number of options to configure sphinx eventually

@agjohnson, could you provide an example? Most sphinx stuff - configurable via conf.py, I think.

@humitos
Copy link
Member

humitos commented Mar 3, 2017

I think i lean towards option 1, perhaps there are others? This would take adding parsing of the sphinx and sphinx['latex'] dictionaries, and then finally the binary parameter.

I also prefer the option 1 since it prepares RTD for the future in case we want to teak a little bit more sphinx or even the latex builder.

@agjohnson, could you provide an example? Most sphinx stuff - configurable via conf.py, I think.

I think the idea of the .readthedocs.yml is to finally get rid of the conf.py or at least most of it. Not sure anyway.

@skirpichev
Copy link
Contributor Author

I think the idea of the .readthedocs.yml is to finally get rid of the conf.py or at least most of it.

@humitos, I hope, it's not...

@skirpichev
Copy link
Contributor Author

skirpichev commented Mar 4, 2017

@agjohnson, thank you for review and sorry for your time. I'll close this pr per readthedocs/readthedocs.org#2362

@skirpichev skirpichev closed this Mar 4, 2017
@agjohnson agjohnson removed the PR: work in progress Pull request is not ready for full review label Mar 4, 2017
@skirpichev skirpichev deleted the pdflatex branch March 4, 2017 11:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants