Spec URL: https://mbriza.fedorapeople.org/mediawriter.spec SRPM URL: https://mbriza.fedorapeople.org/mediawriter-0.0-1.20160802git0054ae4c.fc24.src.rpm Description: Fedora Media Writer is a tool to help users put fedora images on their portable drives Fedora Account System Username: mbriza
I will review it tonight
It's been a week now. Are you going to review the package any time soon, please?
> %define commit 0054ae4cc15ea715148e65d4390fa774c37bb0aa %global commit 0054ae4cc15ea715148e65d4390fa774c37bb0aa %global shortcommit %(c=%{commit}; echo ${c:0:7}) > Version: 0.0 Version: 0 > Release: 1.20160802git0054ae4c%{?dist} Release: 0.1git%{shortcommit}%{?dist} > License: GPLv2 didn't check carefully, but I suppose GPLv2+ > Source0: https://github.com/MartinBriza/MediaWriter/archive/%{commit}.tar.gz Source0: %{url}/archive/%{commit}/%{name}-%{shortcommit}.tar.gz > BuildRequires: libappstream-glib why would you need that? you don't install/validate any of appstream xmls. > Requires: udisks2 actually as part of F25 we replaced udisks2 with storaged, so probably makes sense to fix it. > mkdir %{_target_platform} move to %prep > %{qmake_qt5} PREFIX=/usr .. don't hardcode "/usr", use %{_prefix}. Also not sure if you really use it in upstream. > make %{?_smp_mflags} -C %{_target_platform} %make_build -C %{_target_platform} > ls -l %{buildroot} drop it. > %{_bindir}/mediawriter %{_bindir}/%{name} > %{_libexecdir}/mediawriter/helper then who owns %{_libexecdir}/mediawriter/ ? just use %{_libexecdir}/%{name}/ * Missing BuildRequires: gcc-c++ * I guess all Requires should have %{?_isa}
Hi Igor, thank you for the thorough review. I uploaded new (and fixed) specfile and srpms here: Spec URL: https://mbriza.fedorapeople.org/mediawriter/2/mediawriter.spec SRPM URL: https://mbriza.fedorapeople.org/mediawriter/2/mediawriter-0-0.1git0054ae4.fc24.src.rpm I tended to all your notes while commenting out libappstream (will be used in the future) and still using the PREFIX variable as it actually is used by the project.
> #f24/23 Requires: udisks2 > Requires: storaged%{?_isa} * whitespace detected ;) * you can do something: %if 0%{?fedora} && 0%{?fedora} < 25 Requires: udisks2%{?_isa} %else Requires: storaged%{?_isa} %endif > %{_libexecdir}/%{name} I would prefer to have trailing "/" to be sure that it's directory
> %if 0%{?fedora} && 0%{?fedora} < 2 Noo, I wanted to avoid this. But since you suggested so, it's there. And the other stuff, too. Spec URL: https://mbriza.fedorapeople.org/mediawriter/3/mediawriter.spec SRPM URL: https://mbriza.fedorapeople.org/mediawriter/3/mediawriter-0-0.1git0054ae4.fc24.src.rpm Thank you for the review!
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/mediawriter
mediawriter-0-1.1git728a879.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-3ddbdc6ebb
mediawriter-0-1.1git728a879.fc25 has been pushed to the Fedora 25 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-2016-3ddbdc6ebb
mediawriter-0-1.1git728a879.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.