-
Notifications
You must be signed in to change notification settings - Fork 567
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
Return an error if latex does. #947
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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): | ||
ryanlovett marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I see the issue. Look at the code a few lines above. The first 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 if not latex_rc or … should work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @t-makaro I think optimally There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also thank you for digging in to find the underlying issue. Been needing some eyes on these issues for a bit now. |
||
raise LatexFailed('\n'.join(self._captured_output)) | ||
self.log.info('PDF successfully created') | ||
with open(pdf_file, 'rb') as f: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.