Make travis run all tests (not SKIPPED tests)

[jankatins]

This is a test to see if thats the problem which lead the pdf test not beeing run #350

[flying-sheep]

PDF stuff seems still to be skipped:

• https://travis-ci.org/jupyter/nbconvert/jobs/151307788#L445
• https://travis-ci.org/jupyter/nbconvert/jobs/151307788#L550
• https://travis-ci.org/jupyter/nbconvert/jobs/151307788#L564
• https://travis-ci.org/jupyter/nbconvert/jobs/151307788#L576

[jankatins]

I have the feeling that this might have somthign to do with 1db4edc which changed the command but not the test?

[jankatins]

Looks like no latex is installed on travis?

$ which pdflatex
The command "which pdflatex" exited with 1.
$ which xelatex
The command "which xelatex" exited with 1.

[jankatins]

And now it fails: https://travis-ci.org/jupyter/nbconvert/jobs/151315097#L485

[jankatins]

This now runs all tests in travis (no more SKIPPED tests -> the "purpose" of this PR is therefore accomplished), but that exposed quite a lot of failures: https://travis-ci.org/jupyter/nbconvert/jobs/151317439

[jankatins]

And now we are in trouble:

E           ! LaTeX Error: File `adjustbox.sty' not found.

But according to this SO answer this file isn't in the version of texlive-latex-extra in ubuntu precise which is used on travis.

[takluyver]

Travis does have a beta option to use Trusty (14.04) for builds. I don't know what the queue time is like for those.

https://docs.travis-ci.com/user/trusty-ci-environment/

[takluyver]

Looks like the queue is not too bad, at least at the moment - those builds had started within a couple of minutes of your commit.

[takluyver]

The builds seems to run for a similar amount of time to those on master, which is a good sign if we can get them working.

[takluyver]

OK, now we have reached the same test failure described in #350, which #353 aims to fix.

[jankatins]

This is interesting: the test actually sets ' --PDFExporter.verbose=True' but the test doesn't capture the latex error :-( https://travis-ci.org/jupyter/nbconvert/jobs/151338799#L1688

/home/travis/virtualenv/python3.5.0/lib/python3.5/site-packages/nbconvert/tests/test_nbconvertapp.py:117: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <nbconvert.tests.test_nbconvertapp.TestNbConvertApp testMethod=test_pdf>
parameters = ['--log-level', '0', '--to', 'pdf', 'notebook2', '--PDFExporter.latex_count=1', ...]
ignore_return_code = False, stdin = None
[...]
E               raise LatexFailed('\n'.join(self._captured_output))
E           nbconvert.exporters.pdf.LatexFailed: PDF creating failed, captured latex output:

[jankatins]

I cherrypicked e1f1e11 into this PR, lets see what happens...

[jankatins]

Now it runs into a kernel startup timeout on py3.3: https://travis-ci.org/jupyter/nbconvert/jobs/151344622

???

[takluyver]

That may be a random failure, I've restarted it to see if it passes.

[jankatins]

seems to be, now it's green :-)

[flying-sheep]

seems like my weird choices have uncovered yet another bug 😉

great work fixing it!

[takluyver]

So now, as on #353, we just have to work out if adding the -shell-escape flag is actually the right thing to do.

@michaelpacer @fperez, other latex users - does adding that flag make sense? Is it going to break other things?

[flying-sheep]

it’s not breaking anything, but it disables a sandbox: malicious LaTeX code could do bad things with it.

so the question is if we trust all LaTeX code we encounter, or if adding -shell-escape only for trusted notebooks would be feasible

[jankatins]

My vote (still :-)) goes to removing the minted package: in IRkernel there are two ways to highlight source code of a displayed function (NOT the code in a cell, but things like display(function)): one with a package in R, one with latex (=minted). IMO it would be enough to only use the R stuff. If someone wants special latex function, it's only an overwrite of a repr function away. IMO that's enough for a mostly unused feature (not a lot of use cases to display source code for already defined functions).

[takluyver]

Bad things as in arbitrary code execution? I would say that's not an expected result of running nbconvert to convert a document.

I'd be OK with doing that on trusted notebooks (reusing the trust database we already use for opening notebooks in the browser). But then what do we do with untrusted notebooks?

[flying-sheep]

maybe catch the error in the LaTeX output and give a user friendly warning that this specific notebook can only be converted to PDF if it’s trusted?

[takluyver]

Maybe, but I'm wary of anything that fails on untrusted notebooks, because it's very easy to end up with users going "whatever, I'll trust this so I can do what I want". That's the 'enable macros' curse - you can't rely on users reading a security warning if it's blocking the path to what they want to do.

If minted only works with this sandbox disabled, I'd rather explore @JanSchulz's ideas for how we can do things without it.

[flying-sheep]

yeah, minted calls into pygments and therefore needs to access external commands. \write18 is TeX’ way to call commands and is disabled by default.

without it, TeX can only create files, not execute things. (which can also be used to cause mayhem of course)

we just have to think about some things before deciding this: do we already have output formats that can have arbitrary code execution on creation?

if not i’d agree to removing the minted line again, switch the default of repr.function.highlight to TRUE and add highr to reprs dependencies. i tried to make repr have as little dependencies as possible, but if this has to be changed then it shouldn’t be a problem.

[fperez]

I agree that arbitrary external code execution on format conversion sounds like something we should avoid... @takluyver's comment about the "curse of macros" is on-point.

If users pass --execute they know they are asking the code in the notebook to run, but at least they can assume that it's only that code that runs, so they have a sense of the security perimeter. But having simple PDF conversion trigger third-party code calls would be very surprising. My vote is to avoid it, even at the cost of some loss of functionality.

Given my admittedly limited understanding of the issue, I lean towards @JanSchulz's suggestion of dropping minted altogether.

[takluyver]

And I don't think any of our other export formats allow arbitrary code execution from the notebook (though we haven't closely audited that).

[takluyver]

Or we could do what we already do in kernelspecs, and allow metadata to specify names for a codemirror mode and a pygments lexer to override a more general specifier.

[flying-sheep]

why not just serve it as text/x-r and have notebook and nbconvert handle the displaying/conversion to HTML?

[takluyver]

That's also possible, but it requires them to handle outputs with a much larger range of mimetypes. It's also a breaking change: frontends and tools not aware of the change processing output/notebooks from things that are will show no output (or they'll need a second copy included as text/plain). With my proposal, frontends which aren't aware of the highlighting metadata will still display it as unhighlighted text, without the kernel needing to provide a fallback.

My idea also allows a frontend to distinguish between e.g. an HTML output (text/html) and highlighted HTML source code (text/plain with highlight_as metadata).

[flying-sheep]

or they'll need a second copy included as text/plain

that was the idea. we can then remove that one later

My idea also allows a frontend to distinguish between e.g. an HTML output (text/html) and highlighted HTML source code (text/plain with highlight_as metadata).

good point. still, it isn’t text/plain, so serving it as such is a lie… maybe there’s something better still

[takluyver]

I think sending as text/plain to display source code is a pragmatic approach. Github does this for the 'raw' view of text-based files, including SVG and HTML.

Also, I don't think it's a lie to call source code text/plain; there may be a more specific mime type for it, but the larger category is not inaccurate. The XDG mime database includes a notion of inheritance, and sure enough, text/x-python and text/html both inherit from text/plain.

[flying-sheep]

i’m convinced. i think that highlight_as is a bit too specific though. maybe other consumers might do something else with it.

that metadata describes the “most specific programming/markup language mimetype”, and we should base its name on that. maybe lang-mime/ language-mime/ lang-mimetype/ language-mimetype? or is there precedent for using underscores? i think language-mime is my favourite.

[takluyver]

I don't have a strong opinion on the name; language-mime sounds fine to me. I'll open a new issue for this, so we can get back to the immediate issue here (removing minted).

[takluyver]

#363 is the issue for that feature.

[flying-sheep]

can @JanSchulz just remove it in this PR? then we’ll also see if this runs without -shell-escape and without minted.

i sent a repr PR anyway: IRkernel/repr#77

[mpacer]

I'm curious, why don't we enable a flag when invoking nbconvert that allows it to pass --shell-escape to the XeLaTeX interpreter? That way we could write tests that run on trusted notebooks and allow users to use \write18{} commands which they're likely only going to do if they know what --shell-escape is anyway. I'm on board for by default getting rid of minted which would require this, but it seems like we can keep options open for users who know what including --shell-escape is going to do.

[takluyver]

The command used to invoke Latex is already configurable. I'd be wary of:

1. making it too easy to disable a security feature, especially where the name makes it not obvious that that's what it is.
2. starting down the road of exposing lots of latex options through nbconvert. If you want to use latex in all its glory, you can get a .tex file from nbconvert and run latex separately.

[jankatins]

rebased on top of master: IMO this is ready to merge

[takluyver]

Am I right that sudo: required is necessary in order to use trusty? If so, a comment to that effect would be good; hopefully that will change at some point in the future, and then we can stop requiring sudo again.

[jankatins]

done

[takluyver]

Thanks. Then I think this is fine, but I'm tired, so I'll leave it for others to have a look at in case I'm overlooking something.