Bug 819237
| Summary: | Review Request: vdr-screenshot - Extended screenshot plugin for VDR | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | MartinKG <mgansser> | ||||
| Component: | Package Review | Assignee: | Ville Skyttä <ville.skytta> | ||||
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
| Severity: | unspecified | Docs Contact: | |||||
| Priority: | unspecified | ||||||
| Version: | 17 | CC: | kevin, notting, package-review, ville.skytta, vpvainio | ||||
| Target Milestone: | --- | Flags: | ville.skytta:
fedora-review+
gwync: fedora-cvs+ |
||||
| Target Release: | --- | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Linux | ||||||
| Whiteboard: | |||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||
| Doc Text: | Story Points: | --- | |||||
| Clone Of: | Environment: | ||||||
| Last Closed: | 2012-05-26 12:00:04 UTC | Type: | Bug | ||||
| Regression: | --- | Mount Type: | --- | ||||
| Documentation: | --- | CRM: | |||||
| Verified Versions: | Category: | --- | |||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||
| Embargoed: | |||||||
| Attachments: |
|
||||||
|
Description
MartinKG
2012-05-05 20:57:34 UTC
SRPM URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-markad/vdr-markad-0.1.3-2.20120504git.fc17.src.rpm?a=VpD0aRAPkcg Spec URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-markad/vdr-markad.spec?a=GvSk_pUblyY rpmlint vdr-markad-0.1.3-2.20120504git.fc17.x86_64.rpm vdr-markad.x86_64: I: enchant-dictionary-not-found en_US vdr-markad.x86_64: W: unstripped-binary-or-object /usr/lib64/vdr/libvdr-markad.so.1.7.27 vdr-markad.x86_64: W: unstripped-binary-or-object /usr/bin/markad vdr-markad.x86_64: W: non-standard-uid /etc/vdr/plugins/markad vdr vdr-markad.x86_64: W: non-standard-uid /usr/share/locale/sk_SK/LC_MESSAGES/vdr-markad.mo vdr vdr-markad.x86_64: W: non-standard-uid /usr/share/locale/it_IT/LC_MESSAGES/vdr-markad.mo vdr vdr-markad.x86_64: W: non-standard-uid /usr/share/locale/de_DE/LC_MESSAGES/vdr-markad.mo vdr vdr-markad.x86_64: W: non-standard-uid /usr/share/locale/es_ES/LC_MESSAGES/vdr-markad.mo vdr vdr-markad.x86_64: W: non-standard-uid /usr/share/locale/fi_FI/LC_MESSAGES/vdr-markad.mo vdr vdr-markad.x86_64: W: no-manual-page-for-binary markad 1 packages and 0 specfiles checked; 0 errors, 9 warnings. New Links sorry for the wrong ones. SRPM URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot-0.0.13-1.fc17.src.rpm?a=-f3uJiKPIIA Spec URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot.spec?a=vpx17x7qxMQ rpmlint output: rpmlint vdr-screenshot-0.0.13-1.fc17.x86_64.rpm vdr-screenshot.x86_64: I: enchant-dictionary-not-found en_US vdr-screenshot.x86_64: W: unstripped-binary-or-object /usr/lib64/vdr/libvdr-screenshot.so.1.7.27 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Martin, do you need a sponsor? If yes, be sure to mention it and set the FE-NEEDSPONSOR blocker, see docs at https://fedoraproject.org/wiki/PackageMaintainers/Join#Create_Your_Review_Request https://fedoraproject.org/wiki/Package_Review_Process Be also sure to run rpmlint on source rpm too and fix applicable issues, it reveals e.g.: vdr-screenshot.src:6: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 6) The unstripped-binary-or-object warning needs to be fixed. Haven't looked into it yet, but an usual reason is missing executable permissions on the *.so. Unless someone beats me to it, I'll look into reviewing this later (no sooner than next weekend, though). In the meantime, I also suggest looking into other vdr-* plugin packages in Fedora 17+ and "modernizing" your package like they're done - e.g. no need for so many %globals because that stuff is already defined in vdr-devel's macros (%vdr_*) etc. Ville, yes i need a sponsor ! SRPM URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot-0.0.13-2.fc17.src.rpm?a=hBK69pmnutU Spec URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot.spec?a=rZmVZk1x_CI rpmlint output: rpmlint vdr-screenshot-0.0.13-2.fc17.x86_64.rpm vdr-screenshot.x86_64: I: enchant-dictionary-not-found en_US 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint vdr-screenshot-0.0.13-2.fc17.src.rpm vdr-screenshot.src: I: enchant-dictionary-not-found en_US 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Manually stripping the *.so to "solve" the unstripped-binary-or-object warning is definitely the wrong thing to do, instead it needs to be fixed so that rpmbuild's debuginfo extraction takes care of it. That's what I meant in comment 3, see also https://fedoraproject.org/wiki/Packaging:Debuginfo . I suppose there was nothing wrong with the original package (so the stripping should now be just removed), maybe you just didn't have the redhat-rpm-config package installed on your box? I **strongly** suggest using mock to build packages, not a regular user, see https://fedoraproject.org/wiki/Extras/MockTricks . Also, be sure that your box has the fedora-packager package installed, it pulls in various needed things. I personally would not "fix" the incorrect FSF address issue but rather notify upstream about it, see "rpmlint -I incorrect-fsf-address". This is not a blocker though, if you insist. vdr-devel dependency needs to be >= 1.6.0-41 so that the %vdr_* macros are available. I'm not happy with /tmp being the default path due to possibility of insecure temp file issues. The code uses GrabImageFile from VDR's device.c which seems protected against such issues but I'd be more comfortable with it if the default path wasn't world writable, for example %{vdr_cachedir}/screenshot would sound better to me. (In reply to comment #5) > for example %{vdr_cachedir}/screenshot would sound better to me. That dir should be created and owned by this plugin BTW. redhat-rpm-config and fedora-packager already installed, mock works also.
new rpm package:
regarding "incorrect FSF address" i informed publisher via mail, there is no bug
reporting mechanism.
added patch to store images in /var/cache/vdr/screenshot
cScreenshotConfig::cScreenshotConfig(void) {
- strcpy(sPath, "/tmp");
+ strcpy(sPath, "/var/cache/vdr/screenshot");
removed strip command and comments out debug_package in rpmmacros
SRPM URL:
https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot-0.0.13-3.fc17.src.rpm?a=RBn4RmxYLKI
Spec URL:
https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot.spec?a=3WYsFVZUeYc
rpmlint output:
rpmlint vdr-screenshot-0.0.13-3.fc17.src.rpm
vdr-screenshot.src: I: enchant-dictionary-not-found en_US
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint vdr-screenshot-0.0.13-3.fc17.x86_64.rpm
vdr-screenshot.x86_64: I: enchant-dictionary-not-found en_US
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
New Version SRPM URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot-0.0.14-4.fc17.src.rpm?a=CnF0ZyOupEA Spec URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot.spec?a=buETwd7HCqc rpmlint output: rpmlint vdr-screenshot-0.0.14-4.fc17.src.rpm vdr-screenshot.src: I: enchant-dictionary-not-found en_US 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint vdr-screenshot-0.0.14-4.fc17.x86_64.rpm vdr-screenshot.x86_64: I: enchant-dictionary-not-found en_US 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Created attachment 584481 [details]
Translation Content-Type charset fixes
Content-Type charset of most po/*.po is wrong, causing broken chars in VDR UI. Patch attached, I've already sent it upstream too.
Taking screenshots doesn't actually work with 0.0.14-4 because the /var/cache/vdr/screenshot is not owned by %{vdr_user} but root. Please be sure to test packages yourself before submitting them for review...
README still refers to /tmp, that could be fixed while at it.
To make rpmlint able to check spelling, install the hunspell-en package.
Cosmetic: it's common practice in Fedora to reset the release tag to 1 when updating from upstream version to another, but that's not a blocker.
SRPM URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot-0.0.14-2.fc17.src.rpm?a=ovt8WkmH_3I Spec URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot.spec?a=A6zmPocvTXo hunspell-en is now installed rpmlint output: rpmlint vdr-screenshot-0.0.14-2.fc17.src.rpm vdr-screenshot.src: W: spelling-error Summary(en_US) screenshots -> screen shots, screen-shots, screens hots vdr-screenshot.src: W: spelling-error %description -l en_US mainmenu -> main menu, main-menu, trainmen vdr-screenshot.src: W: spelling-error %description -l en_US yyyymmdd vdr-screenshot.src: W: spelling-error %description -l en_US hhmmss vdr-screenshot.src: W: spelling-error %description -l en_US jpg -> jog, jg, pg 1 packages and 0 specfiles checked; 0 errors, 5 warnings. rpmlint vdr-screenshot-0.0.14-2.fc17.x86_64.rpm vdr-screenshot.x86_64: W: spelling-error Summary(en_US) screenshots -> screen shots, screen-shots, screens hots vdr-screenshot.x86_64: W: spelling-error %description -l en_US mainmenu -> main menu, main-menu, trainmen vdr-screenshot.x86_64: W: spelling-error %description -l en_US yyyymmdd vdr-screenshot.x86_64: W: spelling-error %description -l en_US hhmmss vdr-screenshot.x86_64: W: spelling-error %description -l en_US jpg -> jog, jg, pg vdr-screenshot.x86_64: W: non-standard-uid /usr/share/locale/tr_TR/LC_MESSAGES/vdr-screenshot.mo vdr vdr-screenshot.x86_64: W: non-standard-uid /usr/share/locale/es_ES/LC_MESSAGES/vdr-screenshot.mo vdr vdr-screenshot.x86_64: W: non-standard-uid /usr/share/locale/fi_FI/LC_MESSAGES/vdr-screenshot.mo vdr vdr-screenshot.x86_64: W: non-standard-uid /var/cache/vdr/screenshot vdr vdr-screenshot.x86_64: W: non-standard-uid /usr/share/locale/de_DE/LC_MESSAGES/vdr-screenshot.mo vdr vdr-screenshot.x86_64: W: non-standard-uid /usr/share/locale/ca_ES/LC_MESSAGES/vdr-screenshot.mo vdr vdr-screenshot.x86_64: W: non-standard-uid /usr/share/locale/it_IT/LC_MESSAGES/vdr-screenshot.mo vdr 1 packages and 0 specfiles checked; 0 errors, 12 warnings. (In reply to comment #10) > vdr-screenshot.x86_64: W: non-standard-uid > /usr/share/locale/tr_TR/LC_MESSAGES/vdr-screenshot.mo vdr These errors need fixing, translations should be owned by root, not vdr. Dropping all %defattrs and using plain "%attr(-,%{vdr_user},root) %dir %{vdr_cachedir}/screenshot/" just for the dir would be one way to fix it, or adding one more %defattr(-,root,root,-) at the end - that's the one that ends up affecting stuff passed to %files with -f. Non-blocker, but I'd recommend combining the picture-path and readme patches into one. BTW upstream reported that the *.po patch will be included in the next release. SRPM URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot-0.0.14-3.fc17.src.rpm?a=mpJvUWzKWgc Spec URL: https://www.disk.dsl.o2online.de/FclyPlh/RPMS/VDR/vdr-screenshot/vdr-screenshot.spec?a=yszn7xxxQkE rpmlint output: rpmlint vdr-screenshot-0.0.14-3.fc17.src.rpm vdr-screenshot.src: W: spelling-error Summary(en_US) screenshots -> screen shots, screen-shots, screens hots vdr-screenshot.src: W: spelling-error %description -l en_US mainmenu -> main menu, main-menu, trainmen vdr-screenshot.src: W: spelling-error %description -l en_US yyyymmdd vdr-screenshot.src: W: spelling-error %description -l en_US hhmmss vdr-screenshot.src: W: spelling-error %description -l en_US jpg -> jog, jg, pg 1 packages and 0 specfiles checked; 0 errors, 5 warnings. rpmlint vdr-screenshot-0.0.14-3.fc17.x86_64.rpm vdr-screenshot.x86_64: W: spelling-error Summary(en_US) screenshots -> screen shots, screen-shots, screens hots vdr-screenshot.x86_64: W: spelling-error %description -l en_US mainmenu -> main menu, main-menu, trainmen vdr-screenshot.x86_64: W: spelling-error %description -l en_US yyyymmdd vdr-screenshot.x86_64: W: spelling-error %description -l en_US hhmmss vdr-screenshot.x86_64: W: spelling-error %description -l en_US jpg -> jog, jg, pg vdr-screenshot.x86_64: W: non-standard-uid /var/cache/vdr/screenshot vdr 1 packages and 0 specfiles checked; 0 errors, 6 warnings. (In reply to comment #11) > (In reply to comment #10) > > vdr-screenshot.x86_64: W: non-standard-uid > > /usr/share/locale/tr_TR/LC_MESSAGES/vdr-screenshot.mo vdr > > These errors need fixing, translations should be owned by root, not vdr. > Dropping all %defattrs and using plain "%attr(-,%{vdr_user},root) %dir > %{vdr_cachedir}/screenshot/" just for the dir would be one way to fix it, or > adding one more %defattr(-,root,root,-) at the end - that's the one that ends > up affecting stuff passed to %files with -f. > > Non-blocker, but I'd recommend combining the picture-path and readme patches > into one. > > BTW upstream reported that the *.po patch will be included in the next release. Ville, is the rpm package and the review ok ?. Yes, it's ok, and I'll sponsor you. Let me know your Fedora account name when you've got it and have completed the CLA process (cla_done). Just one final minor detail, no need to submit a new package for this; just please remember to run rpmlint for *all* packages (binary (preferably on an installed package), source, debuginfo) in the future: vdr-screenshot-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/screenshot-0.0.14/screenshot.c A fix would be something like "chmod -c -x screenshot.c" in %prep. New Package SCM Request ======================= Package Name: vdr-screenshot Short Description: Extended screenshot plugin for VDR Owners: martinkg Branches: f16 f17 InitialCC: Careful with the flags - don't touch fedora-review, set fedora-cvs instead; see Wiki for more info. Git done (by process-git-requests). the package built successfully vdr-screenshot-0.0.14-4.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/vdr-screenshot-0.0.14-4.fc17 vdr-screenshot-0.0.14-4.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/vdr-screenshot-0.0.14-4.fc16 vdr-screenshot-0.0.14-4.fc16 has been pushed to the Fedora 16 stable repository. vdr-screenshot-0.0.14-4.fc17 has been pushed to the Fedora 17 stable repository. If problems still persist, please make note of it in this bug report. |