Return an error if latex does. #947
Conversation
LaTeX can error, but still produce a PDF. This ensures the user is aware of the error.
|
Test failures seem to indicate either we already had failures trying to export to PDF, or latex's return code of non-zero means nothing. |
|
Relevant open issue: #896 |
| @@ -175,7 +175,7 @@ def from_notebook_node(self, nb, resources=None, **kw): | |||
| rc = self.run_latex(tex_file) | |||
|
|
|||
| pdf_file = notebook_name + '.pdf' | |||
| if not os.path.isfile(pdf_file): | |||
| if not rc or not os.path.isfile(pdf_file): | |||
There was a problem hiding this comment.
I think I see the issue. Look at the code a few lines above. The first rc = self.run_latex(tex_file) succeeds. Then:
if rc:
rc = self.run_bib(tex_file)runs and fails which, with your change, causes tests to think they are failing.
I think saving the first return code as latex_rc = rc = self.run_latex(tex_file) and then doing
if not latex_rc or …should work.
There was a problem hiding this comment.
@t-makaro I think optimally self.run_bib(tex_file) shouldn't be run if there is no bibliographic data, but I agree that your approach works in those cases. As I understand it, wouldn't it ignore legitimate bibtex failures too?
There was a problem hiding this comment.
Yes, the bibtex should not be run if there is no bibliographic data. I'm not sure how the bibtex integration works. Detecting the existence of a bibtex file may not be enough.
I think that just checking for latex failures would catch/display >95% of errors, and it would not harm any future endeavour to catch legitimate bibtex errors which are not caught now anyway. IMHO, catching only latex errors should be merged for the 5.5 release as it would solve a very long standing issue.
CC @MSeal
There was a problem hiding this comment.
I think it's fair to solve for latex issues first and tackle bibtex later. I too am not familiar with the bibtex integration enough to say how it should work here without some research into the problem.
The plan is to get 5.5 out here in April. If you want to get tests passing and then try tackling the biliographic failure catches in a follow-up thread we can get at least the minimal improvement. If you think you see a clear path to catching both that your confident in I'm ok with that too. Up to you on what you can fix before we release in a week or so. Tests will need to be passing for a merge either way.
There was a problem hiding this comment.
Also thank you for digging in to find the underlying issue. Been needing some eyes on these issues for a bit now.
| @@ -172,12 +172,11 @@ def from_notebook_node(self, nb, resources=None, **kw): | |||
| self.log.info("Building PDF") | |||
| rc = self.run_latex(tex_file) | |||
| if rc: | |||
| rc = self.run_bib(tex_file) | |||
| if rc: | |||
| self.run_bib(tex_file) | |||
There was a problem hiding this comment.
The only problem that I see with removing the if statement is that it means running latex an extra 3 times even if we don't need it. Before the second latex would only run if bibtex was successful.
There was a problem hiding this comment.
@t-makaro I agree.
However, it seems that manually running xelatex on the nbconverted-converted notebook2.ipynb fails (returns non-zero exit code) though it does produce a .pdf file. To me this seems like a bad .tex file to run tests on -- so maybe the conversion from ipynb to tex is incorrect somehow? As I indicated above I think a non-zero return from xelatex should return a LatexFailed exception.
There was a problem hiding this comment.
I think it is reasonable to change the .ipynb used for the test, so that it does not produce a latex error.
There was a problem hiding this comment.
The error occurs in this notebook. The error is in the last markdown cell.
Change:
$$$$
to
$$ $$
and the conversion will be successful. Pandoc butchers that empty math block without the space in the middle.
|
I believe this is now solved by #1053 -- going to close this issue and get the release ball rolling now. |
LaTeX can error, but still produce a PDF. This ensures the user is aware
of the error.