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.

Sign up for GitHub

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

Jump to bottom

Return an error if latex does. #947

Closed
wants to merge 3 commits into from
Closed

Return an error if latex does. #947

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16fe302
Return an error if latex does.
ryanlovett Jan 29, 2019
37495e9
Merge branch 'master' of github.com:jupyter/nbconvert
ryanlovett Apr 4, 2019
4ce84e6
Ignore bibtex return code.
ryanlovett Apr 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions nbconvert/exporters/pdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@t-makaro t-makaro Apr 11, 2019 •
edited
Loading

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.

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
Copy link
Contributor

@t-makaro t-makaro Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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 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.

Copy link
Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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:
Expand Down