https://bugzilla.redhat.com/show_bug.cgi?id=2278420 --- Comment #8 from wojnilowicz <lukasz.wojnilowicz@xxxxxxxxx> --- (In reply to Jerry James from comment #5) > (In reply to wojnilowicz from comment #4) > > Great. Thanks for your fast package review. It may take me a bit longer to > > do the same, because this is my first review ever. > > Oh my. You picked a complicated package for your first review. :-) Note > that you should change the bug status from New to Assigned when you take a > bug for review. I'll do that for you. I did not have much choice. You were the only one looking for swaps recently :) Besides, it's a good learning opportunity for the future. I have like 30 packages to package in a queue. Thanks for changing the bug status for me. > > 1) Did you went through the rpmlint list? Following warnings/errors seem > > reasonable. > > > > python3-jupytext.noarch: W: python-leftover-require python-jupyter-filesystem > > This one is a false positive. The python-jupyter-filesystem package is not > really a python package. It is a filesystem package, providing ownership > for the directories into which the jupyter files are installed. Ok. Could you then change the following comment "# Move the configuration files to where we want them" to something mentioning that you move it to match the python-jupyter-filesystem directory? > > ppython3-jupyterlab-jupytext.noarch: E: backup-file-in-package > > /usr/share/jupyter/labextensions/jupyterlab-jupytext/schemas/jupyterlab- > > jupytext/package.json.orig > > I saw there were backup files in %{python3_sitelib} and removed those, but > missed this one. I have updated the spec file to remove it, too. Ok. > > 2) Is this already solved? > > License file 834.90ed5e1570392532523d.js.LICENSE.txt is not marked as > > %license > > I have updated the package to manage bundled JavaScript licenses as > described here: > https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/ > #_bundled_licenses. That won't make this warning go away, but I believe it > is correct. I would remove it, but who knows if any code needs it. Anyway I believe the jupytext-%{version}-vendor-licenses.txt doesn't match the "License:" field as in "[ ]: License field in the package spec file matches the actual license." The txt file contains among other "BlueOak-1.0.0" and the spec file does not. How should we handle it? > > 3) python-jupytext-doc is provided and I believe it should be > > python3-jupytext-doc. > > I disagree. Back in the days when we had both python 2.x and 3.x, we had > packages with names like python2-foo, python3-foo, and python-foo-doc. The > python3- prefix indicates the presence of python 3.x code, and the > documentation packages applied to both. I think we should continue to > follow that pattern in case there is a python 4 some day. There are many > packages that follow this pattern already; e.g., python-CommonMark-doc, > python-ansicolor-doc, python-cffi-doc, etc. I count 442 such packages > currently in Fedora 40. I agree. I'm on Fedora 39 and "sudo dnf -C se python-*-doc" returns 450 packages while "sudo dnf -C se python3-*-doc" only 38. I think that the chapter at https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_library_naming is not clear enough about this. > > 4) Is this needed in Requires? The project uses pyproject.toml. > > BuildRequires: %{py3_dist setuptools} > > > > 5) Is this needed in Requires? You don't seem to be calling npm. > > BuildRequires: nodejs-npm > > If either one of these is removed, we get a build failure on every > architecture except x86_64. I don't know why x86_64 doesn't need them. > Maybe it's because I generated the vendor tarball on an x86_64 machine, so > whatever step these are needed for was already done for x86_64. Why don't you use nodejs-packaging-bundler from https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_bundling_script ? It produces much smaller dependencies package of 38,6 MB vs yours 102,9 MB with licenses listing included. Additional advantage is that you could drop your custom script. Maybe then a dependency on npm could be dropped as well because the structure of the archive is a bit different as well. > > 6) There seem to be a couple of unowned directories like e.g. /etc/jupyter > > and I believe that's not allowed according to > > https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirectories/ > > Those are the directories owned by python-jupyter-filesystem, which is the > reason this package Requires it. See bug 2264345. Ok. > > 7) I still haven't looked into the package, but aren't those bundled > > libraries that require the FPC exception as in > > "Package contains no bundled libraries without FPC exception."? Have you > > already got this exception? > > # base64-js: MIT > > # buffer: MIT > > # ieee754: BSD-3-Clause > > Nodejs packages are a special case. Bundling is expected. See > https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/ Ok. Shouldn't then jupytext-%{version}-vendor-licenses.txt contain only MIT and BSD-3-Clause? > I have also updated to jupytext 1.16.2, which was just released a few days > ago. There is a new build in the COPR. New URLs: Ok. I found some time to look into your package in more detail, and it looks OK for me. I have further remarks though: 8) You use pytest and "BuildRequires: python3dist(pytest)" is missing as explained at https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_test_dependencies_2 9) Could you "help2man --no-discard-stderr" on jupytext-config to fix "python3-jupytext.noarch: W: no-manual-page-for-binary jupytext-config" ? 10) Out of curiosity, how did you prepare the breakdown of licenses for "%package doc"? Thanks for supporting me in this package review. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2278420 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202278420%23c8 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
