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.

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
Closed

Conversation

ryanlovett
Copy link

LaTeX can error, but still produce a PDF. This ensures the user is aware
of the error.

LaTeX can error, but still produce a PDF. This ensures the user is aware
of the error.
@yuvipanda
Copy link
Contributor

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.

@t-makaro
Copy link
Contributor

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

@t-makaro t-makaro Apr 2, 2019

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.

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

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.

@MSeal
Copy link
Contributor

MSeal commented Aug 6, 2019

I believe this is now solved by #1053 -- going to close this issue and get the release ball rolling now.

@MSeal MSeal closed this Aug 6, 2019
@MSeal MSeal added this to the no action milestone Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants