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

Return an error if latex does. #947

Closed
wants to merge 3 commits into from
+2 −3
Closed

Return an error if latex does. #947

Changes from 1 commit
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
2 changes: 1 addition & 1 deletion nbconvert/exporters/pdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
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