https://bugzilla.redhat.com/show_bug.cgi?id=2283539 --- Comment #9 from Sandro <gui1ty@xxxxxxxxxxxxx> --- (In reply to Ankur Sinha (FranciscoD) from comment #5) > Looking very good. A few things to look at: Thank you for reviewing. I comment inline below. > - Where is README-test-files.md? That will be in dist-git once the package is approved. I uploaded it to: https://gui1ty.fedorapeople.org/review/README-test-files.md I should have done that before or attached the file to the review request, since it answers the questions regarding the test files and why an additional source is required and how I obtained it. > - The GitHub repo, and so the Source, seems to already include the tests > mesh files---is there a reason we're using a different archive/SOURCE for > these instead? I believe the answer is in README-test-files.md? (see above). > - Do the paraview packages also need to be split into -openmpi/-mpich > sub-packages? See my response to Ben's remark below. > - A few unowned directories need to be looked into. Again, since Ben already commented on that, see my reply below. > - python-meshio.spec:126: W: libdir-macro-in-noarch-package > paraview-%{pypi_name} %dir %{_libdir}/paraview > ^ > I think this is OK, since paraview is arched but the plugin files here are > not. Yep. It's just where ParaView is looking for plugins. Annoying, but save to ignore. But the arch part needs fixing as Ben pointed out. (In reply to Ben Beasley from comment #6) > > - Do the paraview packages also need to be split into -openmpi/-mpich sub-packages? > > I think that there is little reason to split the paraview plugins, because > it’s small (in file size and number of files), and more importantly it > doesn’t depend on any of the MPI implementations, so we can install all > three versions/copies of the paraview plugin without dragging in any extra > dependencies. If the plugin linked the MPI libraries, it would be different. I agree. The plugin itself doesn't depend on MPI. It's simply for enabling ParaView to handle additional file formats. In addition to above, it keeps things simple(r) with regards to detecting if ParaView is installed. Though, on revisiting the spec file, this needs fixing. More on that below. (In reply to Ben Beasley from comment #7) > > - python-meshio.spec:126: W: libdir-macro-in-noarch-package paraview-%{pypi_name} %dir %{_libdir}/paraview > > ^ > > I think this is OK, since paraview is arched but the plugin files here are not. > > This does need fixing since %{_libdir} may be either /usr/lib or /usr/lib64 > depending on architecture. Even though the files are arch-independent, the > paths are not. The base package will have to become arched, and then we can > make python3-meshio noarch – and we can also pass "-a" to > %pyproject_extras_subpkg to make the extras metapackages noarch. Thanks. I'll implement that. (In reply to Ben Beasley from comment #8) > > Note: No known owner of > > /usr/lib64/openmpi/lib/paraview/paraview/plugins/meshio/__pycache__, > > /usr/lib64/paraview/paraview/plugins/meshio/__pycache__, > > /usr/lib64/mpich/lib/paraview/paraview/plugins/meshio/__pycache__ > > > > ^ > > Do we need to own this directory? > > Yes, we need > > %dir %{_libdir}/paraview/paraview/plugins/%{pypi_name}/__pycache__ > > and similarly for the other two paraview plugin directories. I believe that will be handled by `%pycached` once the plugin is arched as you pointed out above. Though, I might be misunderstanding the macro[1]. It's the first time I'm using it. Thank you, Ben, for all your input. It's much appreciated. As mentioned above, revisiting the spec file, I believe the following line will not work: Requires: (paraview-%{pypi_name} if paraview) I looked at the MPI flavored packages and they do not depend on it as I initially thought. However, all flavors do depend on `paraview-data`. So, I will be changing that to: Recommends: (paraview-%{pypi_name} if paraview-data) I believe using `Recommends` makes it clearer that the plugin is optional. [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_files_to_include -- 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=2283539 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202283539%23c9 -- _______________________________________________ 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
