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

Add defusedxml as a new standard package #27110

Closed
egourgoulhon opened this issue Jan 24, 2019 · 27 comments
Closed

Add defusedxml as a new standard package #27110

egourgoulhon opened this issue Jan 24, 2019 · 27 comments

Comments

@egourgoulhon
Copy link
Member

The Python package defusedxml
addresses some vulnerabilities of XML parsers.

It is a new dependency of the nbconvert
standard spkg, starting from nbconvert 5.4, to which we upgraded in #26969.

In this ticket, we therefore

  • add defusedxml as a standard spkg, skipping the "optional" phase
  • update nbconvert dependencies

Tarball:
https://files.pythonhosted.org/packages/74/ba/4ba4e89e21b5a2e267d80736ea674609a0a33cc4435a6d748ef04f1f9374/defusedxml-0.5.0.tar.gz


As reported in
this sage-devel post, the absence of defusedxml breaks the Jupyter notebook in Sage 8.7.beta0: opening a notebook file (either a new one or a pre-existing one) results in

500 : Internal Server Error

the reason being

ImportError: No module named defusedxml

Note that anyone using Sage 8.7.beta0 can run

sage --pip install defusedxml

as a simple workaround which will make Jupyter notebooks work again.

CC: @slel

Component: packages: standard

Keywords: jupyter defusedxml

Author: Samuel Lelièvre

Branch/Commit: 6693a6c

Reviewer: Eric Gourgoulhon, Jeroen Demeyer, John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/27110

@slel

This comment has been minimized.

@slel slel changed the title Missing defusedxml module in Jupyter notebook Add defusedxml as a new standard package Jan 24, 2019
@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Jan 24, 2019

comment:2

If anyone is curious, this new dependency of nbconvert was added in
nbconvert pull request 708.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jan 24, 2019

comment:3

Do you plan to write a branch ?

@slel
Copy link
Member

slel commented Jan 25, 2019

Branch: u/slelievre/t/27110

@slel
Copy link
Member

slel commented Jan 25, 2019

comment:5

Branch added.

This is my first time adding an spkg from scratch unsupervised.

I tried to follow the developer manual carefully, specifically this section:

Please review.


New commits:

df7aae9Add defusedxml as a standard spkg

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Jan 25, 2019

Commit: df7aae9

@slel
Copy link
Member

slel commented Jan 25, 2019

comment:6

Does the file build/pkgs/nbconvert/dependencies also need updating?

Currently it just has

$(PYTHON) | pip entrypoints pygments mistune pandocfilters testpath

Compare to the following, found in
nbconvert's setup.py at line 215:

install_requires = setuptools_args['install_requires'] = [
    'mistune>=0.8.1',
    'jinja2',
    'pygments',
    'traitlets>=4.2',
    'jupyter_core',
    'nbformat>=4.4',
    'entrypoints>=0.2.2',
    'bleach',
    'pandocfilters>=1.4.1',
    'testpath',
    'defusedxml',
]
jupyter_client_req = 'jupyter_client>=4.2'

extra_requirements = {
    'test': ['pytest', 'pytest-cov', 'ipykernel', jupyter_client_req],
    'serve': ['tornado>=4.0'],
    'execute': [jupyter_client_req],
    'docs': ['sphinx>=1.5.1',
             'sphinx_rtd_theme',
             'nbsphinx>=0.2.12',
             'sphinxcontrib_github_alt',
             'ipython',
             jupyter_client_req,
             ],
}

@egourgoulhon
Copy link
Member Author

comment:7

Replying to @slel:

Please review.

Thanks for having added the package! After a pull of your branch + download of the tarball in the upstream folder, running make successfully installs the package (after the build of the documentation though, I don't know if this normal, i.e. I would expect all packages to be installed before starting building the documentation, but I may be wrong on this). Then Jupyter runs normally. So I would vote for a positive review.

@jdemeyer
Copy link

Author: Samuel Lelièvre

@jdemeyer
Copy link

comment:9

Replying to @slel:

Does the file build/pkgs/nbconvert/dependencies also need updating?

I think it's best to do this, yes.

@jdemeyer
Copy link

Reviewer: Eric Gourgoulhon, Jeroen Demeyer

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2019

Changed commit from df7aae9 to ee2d8da

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

ee2d8daUpdate nbconvert dependencies

@slel
Copy link
Member

slel commented Jan 27, 2019

comment:11

Update nbconvert dependencies using those in install_requires in nbconvert's setup.py.

@slel

This comment has been minimized.

@seblabbe
Copy link
Contributor

comment:13

I just noticed that the version is hardcoded in the file checksums.ini, citing the above link from the reference manual: "Sage internally replaces the VERSION substring with the content of package-version.txt."

@jhpalmieri
Copy link
Member

Changed branch from u/slelievre/t/27110 to u/jhpalmieri/t/27110

@jhpalmieri
Copy link
Member

Changed commit from ee2d8da to 6693a6c

@jhpalmieri
Copy link
Member

comment:15

This should fix it.


New commits:

6693a6ctrac 27110: don't hardcode version in checksums.ini

@slel
Copy link
Member

slel commented Jan 30, 2019

comment:16

Thanks for catching this.

@slel
Copy link
Member

slel commented Jan 30, 2019

Changed author from Samuel Lelièvre to Samuel Lelièvre, John Palmieri

@jhpalmieri
Copy link
Member

comment:17

I don't know that I really deserve authorship credit for that.

@jdemeyer
Copy link

Changed author from Samuel Lelièvre, John Palmieri to Samuel Lelièvre

@jdemeyer
Copy link

Changed reviewer from Eric Gourgoulhon, Jeroen Demeyer to Eric Gourgoulhon, Jeroen Demeyer, John Palmieri

@vbraun
Copy link
Member

vbraun commented Feb 3, 2019

Changed branch from u/jhpalmieri/t/27110 to 6693a6c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants