Bug 2162694 - Review Request: python-nbclassic - Jupyter Notebook as a Jupyter Server Extension
Summary: Review Request: python-nbclassic - Jupyter Notebook as a Jupyter Server Exten...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Miro Hrončok
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-01-20 14:15 UTC by Lumír Balhar
Modified: 2023-01-30 14:20 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-01-30 14:20:39 UTC
Type: ---
Embargoed:
mhroncok: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 5322650 to 5361948 (2.51 KB, patch)
2023-01-30 12:15 UTC, Jakub Kadlčík
no flags Details | Diff

Description Lumír Balhar 2023-01-20 14:15:25 UTC
Spec URL: https://lbalhar.fedorapeople.org/python-nbclassic.spec
SRPM URL: https://lbalhar.fedorapeople.org/python-nbclassic-0.4.8-1.fc37.src.rpm
Description: This project prepares for a future where JupyterLab and other
frontends switch to Jupyter Server for their Python Web application
backend. Using this package, users can launch Jupyter NbClassic,
JupyterLab and other frontends side-by-side on top of
the new Python server backend.
Fedora Account System Username: lbalhar

This package is needed for Jupyter Notebook 6.5.2. I'm testing them together in COPR: https://copr.fedorainfracloud.org/coprs/lbalhar/notebook6/builds/

Comment 2 Lumír Balhar 2023-01-20 15:07:26 UTC
I forget to specify all the bundled JS libs. I'm looking for a way how to do it.

Comment 3 Miro Hrončok 2023-01-20 15:21:57 UTC
Apart from the JS thing: This package should require the python-jupyter-filesystem package:

$ repoquery -q --repo=rawhide -f /etc/jupyter/jupyter_server_config.d/
python-jupyter-filesystem-0:5.1.0-1.fc38.noarch

Comment 4 Lumír Balhar 2023-01-23 12:10:27 UTC
So, bundled JS libs are now correctly provided and those available in Fedora are specified as requirements. The package now also requires python-jupyter-filesystem.

I did a rebuild in COPR and it still works fine with the latest notebook 6.5 on top of it. I've tested Python, markdown, and latex and they all work. Also, there are no suspicious warnings in the browser console.

Comment 5 Miro Hrončok 2023-01-23 12:36:10 UTC
The notebook package patched the bundled moment to fix CVE-2022-31129. I believe this contains an old version of mement as well which might require keeping that patch.

I'd also strongly consider not unbundling any of the javascript, rebundling it once it is retired from Fedora has proven to be quite cumbersome in Fedora, see https://src.fedoraproject.org/rpms/python-notebook/c/89be135877952b931c3cb6d736a55eb69f6038d9?branch=rawhide

Comment 6 Lumír Balhar 2023-01-23 14:05:33 UTC
(In reply to Miro Hrončok from comment #5)
> The notebook package patched the bundled moment to fix CVE-2022-31129. I
> believe this contains an old version of mement as well which might require
> keeping that patch.

This problem is already fixed upstream. They should release a new version soon. We can wait for it. https://github.com/jupyter/nbclassic/pull/186

> I'd also strongly consider not unbundling any of the javascript, rebundling
> it once it is retired from Fedora has proven to be quite cumbersome in
> Fedora, see
> https://src.fedoraproject.org/rpms/python-notebook/c/
> 89be135877952b931c3cb6d736a55eb69f6038d9?branch=rawhide

Okay, I'm gonna unbundle just the fonts.

Comment 7 Miro Hrončok 2023-01-24 15:34:44 UTC
I've looked at python-notebook spec and there are 2 things in there that appear to still be related here:


$ rpm -qlp python3-nbclassic-0.4.8-1.fc38.noarch.rpm | grep mathjaxutils
/usr/lib/python3.11/site-packages/nbclassic/static/base/js/mathjaxutils.js
/usr/lib/python3.11/site-packages/nbclassic/static/notebook/js/mathjaxutils.js

This means https://src.fedoraproject.org/rpms/python-notebook/blob/rawhide/f/0001-Use-MathJax-TeX-fonts-rather-than-STIXWeb.patch and the reationale in the python-notebook spec probably still stands.




$ rpm -qlp python3-nbclassic-0.4.8-1.fc38.noarch.rpm | grep i18n
/usr/lib/python3.11/site-packages/nbclassic/i18n
/usr/lib/python3.11/site-packages/nbclassic/i18n/__init__.py
/usr/lib/python3.11/site-packages/nbclassic/i18n/__pycache__
/usr/lib/python3.11/site-packages/nbclassic/i18n/__pycache__/__init__.cpython-311.opt-1.pyc
/usr/lib/python3.11/site-packages/nbclassic/i18n/__pycache__/__init__.cpython-311.pyc
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES/nbclassic.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES/nbclassic.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES/nbjs.json
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES/nbjs.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES/nbui.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/fr_FR/LC_MESSAGES/nbui.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES/nbclassic.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES/nbclassic.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES/nbjs.json
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES/nbjs.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES/nbui.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/ja_JP/LC_MESSAGES/nbui.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/nl
/usr/lib/python3.11/site-packages/nbclassic/i18n/nl/LC_MESSAGES
/usr/lib/python3.11/site-packages/nbclassic/i18n/nl/LC_MESSAGES/nbclassic.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/nl/LC_MESSAGES/nbjs.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/nl/LC_MESSAGES/nbui.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES/nbclassic.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES/nbclassic.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES/nbjs.json
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES/nbjs.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES/nbui.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/ru_RU/LC_MESSAGES/nbui.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES/nbclassic.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES/nbclassic.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES/nbjs.json
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES/nbjs.po
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES/nbui.mo
/usr/lib/python3.11/site-packages/nbclassic/i18n/zh_CN/LC_MESSAGES/nbui.po
/usr/lib/python3.11/site-packages/nbclassic/static/base/js/i18n.js

This means the %lang(...) thing and .po fils removal from the python-notebook spec needs to be added here as well.

Comment 8 Lumír Balhar 2023-01-24 16:00:43 UTC
The patch seems to work fine here and I've copied some parts of the %install and %files sections. So it should be fixed now.

Comment 9 Lumír Balhar 2023-01-27 08:45:17 UTC
There is a new release 0.5.1 where the bundled version of moment js is upgraded so the downstream CVE fix is no longer needed.

I'd say that this is ready or at least I'm not aware of any remaining issues.

Spec URL: https://lbalhar.fedorapeople.org/python-nbclassic.spec
SRPM URL: https://lbalhar.fedorapeople.org/python-nbclassic-0.5.1-1.fc37.src.rpm

Comment 12 Miro Hrončok 2023-01-27 20:06:45 UTC
# Originally bundled libraries
Requires:       fontawesome-fonts
Requires:       fontawesome-fonts-web

Nitpick: The comment should probably say "fonts" rather than "libraries" now.

----------

A very small nitpick: %generate_buildrequires could use an extra empty line above it to follow the convention of the rest of the specfile.

----------

What is the license of the bundled JavaScript libraries? It should probably be included in the License tag (I know it wasn't done in the notebook package) :(

See for example https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2162694-python-nbclassic/fedora-rawhide-x86_64/05322649-python-nbclassic/fedora-review/licensecheck.txt

----------

Fedora-Review complains:

- Package installs a %{name}.desktop using desktop-file-install or desktop-
  file-validate if there is such a file.

This is correct, please run desktop-file-validate ...desktop in %check, as the installation is done by %pyproject_install.

python-notebook has:
	
  BuildRequires: desktop-file-utils
  ...
  %check
  ...
  desktop-file-validate %{buildroot}%{_datadir}/applications/jupyter-notebook.desktop

----------

rpmlint complains:

  python3-nbclassic.noarch: E: zero-length /usr/lib/python3.11/site-packages/nbclassic/bundler/tests/resources/subdir/subsubdir/.gitkeep
  python3-nbclassic.noarch: W: hidden-file-or-dir /usr/lib/python3.11/site-packages/nbclassic/bundler/tests/resources/subdir/subsubdir/.gitkeep
  python3-nbclassic.noarch: W: files-duplicate /usr/lib/python3.11/site-packages/nbclassic/bundler/tests/resources/subdir/test_file.txt /usr/lib/python3.11/site-packages/nbclassic/bundler/tests/resources/another_subdir/test_file.txt

I guess the nbclassic/bundler/tests folder would also benefit from being removed entirely. There might be others.

python-notebook has:

  # Remove packaged tests
  rm -rv $(find %{buildroot}%{python3_sitelib}/notebook -type d -name tests)

---------

Curiosity: The package requires (python3.11dist(jupyter-server) >= 1.17 with python3.11dist(jupyter-server) >= 1.8)

I found that a bit odd as 1.8 > 1.17 anyway. Looks like an upstream oversight. https://github.com/jupyter/nbclassic/pull/208 (No need to change the package, just heads up.)

Comment 13 Lumír Balhar 2023-01-30 12:01:42 UTC
All comments addressed, thanks!

Spec URL: https://lbalhar.fedorapeople.org/python-nbclassic.spec
SRPM URL: https://lbalhar.fedorapeople.org/python-nbclassic-0.5.1-1.fc37.src.rpm

Comment 14 Jakub Kadlčík 2023-01-30 12:15:44 UTC
Created attachment 1941077 [details]
The .spec file difference from Copr build 5322650 to 5361948

Comment 16 Miro Hrončok 2023-01-30 12:46:41 UTC
Package APPROVED. The following opinionated suggestions are optional:



> BuildRequires: desktop-file-utils

This could use an extra space before desktop-file-utils to align with the previous BuildRequires in the spec.


> # Main package is BSD-3

Using the actual SPDX tags in the comments makes the License tag easier to review. BSD-3-Clause in this case.

The whole License block would be a bit easier to read if it started and ended with an empty line.


> URL:            http://jupyter.org

Suggestion: https://jupyter.org (HTTPS)

Comment 17 Fedora Admin user for bugzilla script actions 2023-01-30 14:03:40 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-nbclassic


Note You need to log in before you can comment on or make changes to this bug.