Bug 1051665
| Summary: | Review Request: vdr-skinnopacity - A highly customizable native true color skin for the VDR | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | MartinKG <mgansser> |
| Component: | Package Review | Assignee: | Rahul Sundaram <metherid> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | anto.trande, besser82, leamas.alec, metherid, package-review |
| Target Milestone: | --- | Flags: | metherid:
fedora-review+
gwync: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | vdr-skinnopacity-1.0.3-7.20131221git0b29805.fc20 | Doc Type: | Bug Fix |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2014-01-15 19:10:57 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
MartinKG
2014-01-10 20:19:16 UTC
Spec URL: http://martinkg.fedorapeople.org/Review/SPECS/vdr-skinnopacity.spec SRPM URL: http://martinkg.fedorapeople.org/Review/SRPMS/vdr-skinnopacity-1.0.3-2.0b29805.fc20.src.rpm %changelog * Sat Jan 11 2014 Martin Gansser <martinkg> - 1.0.3-2.0b29805 - added themes and themeconfigs file - added compiler flags in build section - removed additional localization install section - corrected path to README in skinnopacity.conf @rahul thank you for taking the review. Spec URL: http://martinkg.fedorapeople.org/Review/SPECS/vdr-skinnopacity.spec SRPM URL: http://martinkg.fedorapeople.org/Review/SRPMS/vdr-skinnopacity-1.0.3-3.0b29805.fc20.src.rpm %changelog * Sun Jan 12 2014 Martin Gansser <martinkg> - 1.0.3-3.0b29805 - removed additional icons install section Cursory first look: * Is there any particular reason the conf file is not part of upstream? https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment * I would use %make_install instead of make install DESTDIR=$RPM_BUILD_ROOT * Might consider using %{buildroot} for consistency Why is this noreplace? if it is not supposed to be edited by users, should it be in /etc? %config(noreplace) %{_sysconfdir}/sysconfig/vdr-plugins.d/skinnopacity.conf (In reply to Rahul Sundaram from comment #4) > Cursory first look: > > * Is there any particular reason the conf file is not part of upstream? > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/ > Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment Configuration files for plugin parameters. These are Fedora specific and not in upstream. In all other vdr plugins the conf-file is also not in the upstream, and it's not a blocker. > * I would use %make_install instead of make install DESTDIR=$RPM_BUILD_ROOT agreed, done. > * Might consider using %{buildroot} for consistency agreed, done. > Why is this noreplace? if it is not supposed to be edited by users, should > it be in /etc? no, it must be in /etc/sysconfig/vdr-plugins.d/ which is required by vdr and it must not be overwritten. > > %config(noreplace) %{_sysconfdir}/sysconfig/vdr-plugins.d/skinnopacity.conf Spec URL: http://martinkg.fedorapeople.org/Review/SPECS/vdr-skinnopacity.spec SRPM URL: http://martinkg.fedorapeople.org/Review/SRPMS/vdr-skinnopacity-1.0.3-4.0b29805.fc20.src.rpm %changelog * Tue Jan 14 2014 Martin Gansser <martinkg> - 1.0.3-4.0b29805 - used %%make_install instead of make install DESTDIR... - using %{buildroot} for consistency rpmlint complains about the use of macro in the changelog. so you might want to drop % or use %% license according to the README is GPLv2+ and not GPL+ /etc/vdr/themes/ doesn't seem to be appropriate location for themes or any non user modifiable configuration files. Also Fedora licensing guideline recommend that source files should have copyright headers clearly stating the author and licensing detail although this isn't a blocker, I would recommend getting in touch with upstream to get this updated (In reply to Rahul Sundaram from comment #6) > rpmlint complains about the use of macro in the changelog. so you might > want to drop % or use %% done > license according to the README is GPLv2+ and not GPL+ done > /etc/vdr/themes/ doesn't seem to be appropriate location for themes or any > non user modifiable configuration files. changed in %install section, as it has already been made in both vdr repo packets. vdr-skinsoppalusikka.spec: install -pm 644 themes/*.theme $RPM_BUILD_ROOT%{vdr_vardir}/themes/ vdr-skinenigmang.spec: install -pm 644 themes/*.theme $RPM_BUILD_ROOT%{vdr_vardir}/themes %install # make install would install the themes under /etc, let's not use that make install-lib install-i18n install-icons DESTDIR=$RPM_BUILD_ROOT # install the themes to the custom location used in Fedora install -dm 755 $RPM_BUILD_ROOT%{vdr_vardir}/themes install -dm 755 $RPM_BUILD_ROOT%{vdr_vardir}/plugins/skinnopacity/themeconfigs install -pm 644 themes/*.theme $RPM_BUILD_ROOT%{vdr_vardir}/themes/ install -pm 644 conf/theme-* $RPM_BUILD_ROOT%{vdr_vardir}/plugins/skinnopacity/themeconfigs/ https://dl.dropboxusercontent.com/s/ta395xq9iv9hary/vdr-skinnopacity.spec https://dl.dropboxusercontent.com/s/ibzot9jb63cgc1p/vdr-skinnopacity-1.0.3-5.0b29805.fc20.src.rpm %changelog * Wed Jan 15 2014 Martin Gansser <martinkg> - 1.0.3-5.0b29805 - removed macro in changelog - corrected license type to GPLv2+ - installed themes and themeconfigs to the custom location used in Fedora (In reply to Rahul Sundaram from comment #7) > Also Fedora licensing guideline recommend that source files should have > copyright headers clearly stating the author and licensing detail although > this isn't a blocker, I would recommend getting in touch with upstream to > get this updated # informed upstream to put copyright and licensing details in source files # http://projects.vdr-developer.org/issues/1679 Please add explicit instructions on generating the tarball https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control Do this change before checking in the latest spec file but I will approve this as it is a minor change. ====== APPROVED ===== Also make sure you are following the naming guidelines for snapshots https://fedoraproject.org/wiki/Packaging:NamingGuidelines#SnapshotPackages New Package SCM Request ======================= Package Name: vdr-skinnopacity Short Description: A highly customizable native true color skin for the VDR Owners: martinkg Branches: f20 f21 InitialCC: Git done (by process-git-requests). f21 not branched yet, devel is automatic. vdr-skinnopacity has been built successfully on fc20 and fc21. vdr-skinnopacity-1.0.3-5.20131221git0b29805.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/vdr-skinnopacity-1.0.3-5.20131221git0b29805.fc20 vdr-skinnopacity-1.0.3-6.20131221git0b29805.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/vdr-skinnopacity-1.0.3-6.20131221git0b29805.fc20 vdr-skinnopacity-1.0.3-7.20131221git0b29805.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/vdr-skinnopacity-1.0.3-7.20131221git0b29805.fc20 vdr-skinnopacity-1.0.3-7.20131221git0b29805.fc20 has been pushed to the Fedora 20 stable repository. If problems still persist, please make note of it in this bug report. |