Hello hope you are doing well. I have been packaging whipper, a fork of the unmaintained morituri, on copr during development and the project has reached a point where I would like to try and get it into Fedora as an official package. This is my first package so would need a sponsor. I hope I have not done anything majorly wrong. Development is targeting libcdio-paranoia 10.2+0.94+1 and above which Adrian Reber thankfully updated in rawhide for me. Spec URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00708563-whipper/whipper.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00708563-whipper/whipper-0.6.0-1.fc28.src.rpm Description: This application is a CD-DA ripper focused on accuracy over speed. Fedora Account System Username: mruszczyk
Just preliminary comments: - Split your Requires on multiple lines - Is it really necessary to split the python part into a separate library package? It doesn't seem to be used as a library, only as a binary. - Python2 is EOL soon, it would be better to build with Python 3 instead, unless it's incompatible. - add a comment to explain why the patch is needed
(In reply to Robert-André Mauchin from comment #1) Thank you very much for the comments. > Just preliminary comments: > > - Split your Requires on multiple lines Done. > - Is it really necessary to split the python part into a separate library > package? It doesn't seem to be used as a library, only as a binary. I did this after asking a few questions in the #fedora-devel irc. They seemed to indicate that it was abnormal to not split python packages this way to make future python 3 support easier to setup. Is that incorrect? > - Python2 is EOL soon, it would be better to build with Python 3 instead, > unless it's incompatible. Currently the application is incompatible. There is a bug filed in github here (https://github.com/JoeLametta/whipper/issues/78) but no major work has been put into it yet. > - add a comment to explain why the patch is needed Sure thing. While I rebuild the package, the patch moves the accuraterip-checksum binary from /usr/local to /usr/bin to comply with the packaging guidelines https://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout
Spec URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00708655-whipper/whipper.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00708655-whipper/whipper-0.6.0-2.fc28.src.rpm Description: This application is a CD-DA ripper focused on accuracy over speed. Fedora Account System Username: mruszczyk Updated spec and srpm for the comments above.
Spec looks good, will run fedora-review soon.
Spec URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00711948-whipper/whipper.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00711948-whipper/whipper-0.6.0-3.fc28.src.rpm Description: CD-DA ripper focused on accuracy over speed. Fedora Account System Username: mruszczyk (In reply to William Moreno from comment #4) > Spec looks good, will run fedora-review soon. Thank you very much for your time, I hope to hear from you soon. I've had to update the spec file above because I was adding the incorrect gobject dependency. It's fixed in the latest version on the copr repo and linked above.
> > - Is it really necessary to split the python part into a separate library > > package? It doesn't seem to be used as a library, only as a binary. > > I did this after asking a few questions in the #fedora-devel irc. They > seemed to indicate that it was abnormal to not split python packages this > way to make future python 3 support easier to setup. Is that incorrect? > It is not common in Fedora packaging to split a python package in a %{name} package and a python2-%{name} subpackage, the only reason to do dat is if the python2-%{name} is a module importable and usefull without the executable files in %{name}, but this are few bites of direference for the user point of view.
Package Review ============== You are requering gobjects, is whipper a command line tool or provides a grafical user interface? If so work with upstream to include a desktop file and appdata.xml file. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/lib/python2.7/site-packages/whipper, /usr/lib/python2.7/site-packages/whipper-0.6.0-py2.7.egg-info [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib/python2.7/site- packages/whipper, /usr/lib/python2.7/site-packages/whipper-0.6.0-py2.7 .egg-info Update %files to: %{python2_sitelib}/whipper-%{version}-py2.7.egg-info %{python2_sitelib}/whipper This way you own the directory and not only the lifes on it. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [x]: Package does not own files or directories owned by other packages. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [x]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size [!]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local Python: [x]: Package contains BR: python2-devel or python3-devel [x]: Binary eggs must be removed in %prep ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Requires -------- whipper (rpmlib, GLIBC filtered): /usr/bin/python2 libc.so.6()(64bit) libsndfile.so.1()(64bit) libsndfile.so.1(libsndfile.so.1.0)(64bit) python2-whipper rtld(GNU_HASH) whipper-debuginfo (rpmlib, GLIBC filtered): whipper-debugsource (rpmlib, GLIBC filtered): python2-whipper (rpmlib, GLIBC filtered): cdrdao flac libcdio-paranoia pycdio pygobject2 python(abi) python2-CDDB python2-musicbrainzngs python2-mutagen python2-requests sox Provides -------- whipper: whipper whipper(x86-64) whipper-debuginfo: debuginfo(build-id) whipper-debuginfo whipper-debuginfo(x86-64) whipper-debugsource: whipper-debugsource whipper-debugsource(x86-64) python2-whipper: python-whipper python-whipper(x86-64) python2-whipper python2-whipper(x86-64) python2.7dist(whipper) python2dist(whipper) Source checksums ---------------- https://github.com/JoeLametta/whipper/archive/v0.6.0.tar.gz : CHECKSUM(SHA256) this package : c1919f1a99638e2649bf81bbb30f8a8b4509ab3e1a97b04346ba0b5c8fac12ea CHECKSUM(SHA256) upstream package : c1919f1a99638e2649bf81bbb30f8a8b4509ab3e1a97b04346ba0b5c8fac12ea
Spec URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-27-x86_64/00714229-whipper/whipper.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-27-x86_64/00714229-whipper/whipper-0.6.0-4.fc27.src.rpm Description: CD-DA ripper focused on accuracy over speed. Fedora Account System Username: mruszczyk (In reply to William Moreno from comment #6) > > > - Is it really necessary to split the python part into a separate library > > > package? It doesn't seem to be used as a library, only as a binary. > > > > I did this after asking a few questions in the #fedora-devel irc. They > > seemed to indicate that it was abnormal to not split python packages this > > way to make future python 3 support easier to setup. Is that incorrect? > > > > It is not common in Fedora packaging to split a python package in a %{name} > package and a python2-%{name} subpackage, the only reason to do dat is if > the python2-%{name} is a module importable and usefull without the > executable files in %{name}, but this are few bites of direference for the > user point of view. Thank you very much for the feedback. I have restructured the spec file to only generate a single binary package. (In reply to William Moreno from comment #7) > Package Review > ============== > > You are requering gobjects, is whipper a command line tool or provides a > grafical user interface? If so work with upstream to include a desktop file > and appdata.xml file. > > [!]: Package requires other packages for directories it uses. > Note: No known owner of /usr/lib/python2.7/site-packages/whipper, > /usr/lib/python2.7/site-packages/whipper-0.6.0-py2.7.egg-info > [!]: Package must own all directories that it creates. > Note: Directories without known owners: /usr/lib/python2.7/site- > packages/whipper, /usr/lib/python2.7/site-packages/whipper-0.6.0-py2.7 > .egg-info > > Update %files to: > %{python2_sitelib}/whipper-%{version}-py2.7.egg-info > %{python2_sitelib}/whipper > > This way you own the directory and not only the lifes on it. The application is a command line tool and does not provide a graphical user interface. The gobject requirement is still in the tree for now but work is being done to eliminate it. I have adjusted the files section to properly take ownership of the directories. Please let me know if there is anything else I need to change at this time.
Spec URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00714828-whipper/whipper.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mruszczyk/whipper/fedora-rawhide-x86_64/00714828-whipper/whipper-0.6.0-5.fc28.src.rpm Description: CD-DA ripper focused on accuracy over speed. Fedora Account System Username: mruszczyk I had to update the spec once more, I was missing python2-setuptools.
From the build log %make_build -C src + /usr/bin/make -O -j6 -C src make: Entering directory '/builddir/build/BUILD/whipper-0.6.0/src' accuraterip-checksum build options: CFLAGS = -g -std=c99 LDFLAGS = -lsndfile CC = cc make: Leaving directory '/builddir/build/BUILD/whipper-0.6.0/src' make: Entering directory '/builddir/build/BUILD/whipper-0.6.0/src' CC accuraterip-checksum.c make: Leaving directory '/builddir/build/BUILD/whipper-0.6.0/src' make: Entering directory '/builddir/build/BUILD/whipper-0.6.0/src' CC -o accuraterip-checksum make: Leaving directory '/builddir/build/BUILD/whipper-0.6.0/src' and %make_install -C src are not necesaries because %py2_build and %py2_install do the work to put the executable file in %{bindir} running install_scripts Installing whipper script to /builddir/build/BUILDROOT/whipper-0.6.0-5.fc28.x86_64/usr/bin Remove those two lines and this package will ready to go.
Also note that for Fedora 29 you will need to add BuildRequires: gcc https://fedoraproject.org/wiki/Changes/Remove_GCC_from_BuildRoot
(In reply to William Moreno from comment #10) > From the build log > > %make_build -C src > > + /usr/bin/make -O -j6 -C src > make: Entering directory '/builddir/build/BUILD/whipper-0.6.0/src' > accuraterip-checksum build options: > CFLAGS = -g -std=c99 > LDFLAGS = -lsndfile > CC = cc > make: Leaving directory '/builddir/build/BUILD/whipper-0.6.0/src' > make: Entering directory '/builddir/build/BUILD/whipper-0.6.0/src' > CC accuraterip-checksum.c > make: Leaving directory '/builddir/build/BUILD/whipper-0.6.0/src' > make: Entering directory '/builddir/build/BUILD/whipper-0.6.0/src' > CC -o accuraterip-checksum > make: Leaving directory '/builddir/build/BUILD/whipper-0.6.0/src' > > and %make_install -C src are not necesaries because %py2_build and > %py2_install do the work to put the executable file in %{bindir} > > running install_scripts > Installing whipper script to > /builddir/build/BUILDROOT/whipper-0.6.0-5.fc28.x86_64/usr/bin > > Remove those two lines and this package will ready to go. Thank you very much for the advice. I am a little bit confused however. Removing either line has the package fail to build. Those lines are building a separate c binary called accuraterip-checksum https://github.com/JoeLametta/whipper/tree/master/src. As far as I can tell %py2_build and %py2_install do not touch that directory at all. If I missed something about how to execute the python2 macros I would love to find out. (In reply to William Moreno from comment #11) > Also note that for Fedora 29 you will need to add BuildRequires: gcc > > https://fedoraproject.org/wiki/Changes/Remove_GCC_from_BuildRoot Thank you, I had seen this, just did not want to edit the package another time during this review.
You are rigth, this package build some c code.
OK I am fine with this package and have added you as a member of the Fedora Packager group in FAS. If you need help to request your new package, import the package to the distribution and build it please ping me.
(fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/whipper
See the dos about setup.py you must add a setup.cfg with: [build_ext] inplace=1 To build c extensions in place, a good patch to send upstream.
whipper-0.6.0-6.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-55d9cc8fb4
whipper-0.6.0-6.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-55d9cc8fb4
whipper-0.6.0-6.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.