Bug 819237 - Review Request: vdr-screenshot - Extended screenshot plugin for VDR
Review Request: vdr-screenshot - Extended screenshot plugin for VDR
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
17
Unspecified Linux
unspecified Severity unspecified
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-05 16:57 EDT by MartinKG
Modified: 2012-06-27 23:24 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-26 08:00:04 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ville.skytta: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Translation Content-Type charset fixes (1.10 KB, patch)
2012-05-14 17:22 EDT, Ville Skyttä
no flags Details | Diff

  None (edit)
Description MartinKG 2012-05-05 16:57:34 EDT
Hi,

With this plugin you can take still images of your screen. After installing
the plugin, a new mainmenu entry "Screenshot" will show up. Each time you
select this item, a file /tmp/title-yyyymmdd-hhmmss.jpg will be created,
where title is the current transmission or the recording currently replayed.

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.
Comment 1 MartinKG 2012-05-06 14:28:08 EDT
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.
Comment 2 MartinKG 2012-05-07 02:25:04 EDT
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.
Comment 3 Ville Skyttä 2012-05-07 16:52:55 EDT
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.
Comment 4 MartinKG 2012-05-08 13:55:51 EDT
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.
Comment 5 Ville Skyttä 2012-05-12 16:00:41 EDT
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.
Comment 6 Ville Skyttä 2012-05-12 16:03:24 EDT
(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.
Comment 7 MartinKG 2012-05-12 18:45:08 EDT
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.
Comment 8 MartinKG 2012-05-13 14:26:55 EDT
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.
Comment 9 Ville Skyttä 2012-05-14 17:22:39 EDT
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.
Comment 10 MartinKG 2012-05-15 12:24:58 EDT
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.
Comment 11 Ville Skyttä 2012-05-15 14:10:46 EDT
(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.
Comment 12 MartinKG 2012-05-15 14:48:31 EDT
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.
Comment 13 MartinKG 2012-05-20 11:22:42 EDT
(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 ?.
Comment 14 Ville Skyttä 2012-05-20 17:10:16 EDT
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.
Comment 15 MartinKG 2012-05-21 13:15:52 EDT
New Package SCM Request
=======================
Package Name: vdr-screenshot
Short Description: Extended screenshot plugin for VDR
Owners: martinkg
Branches: f16 f17
InitialCC:
Comment 16 Ville Skyttä 2012-05-21 16:49:09 EDT
Careful with the flags - don't touch fedora-review, set fedora-cvs instead; see Wiki for more info.
Comment 17 Gwyn Ciesla 2012-05-22 08:22:42 EDT
Git done (by process-git-requests).
Comment 18 MartinKG 2012-05-26 08:00:04 EDT
the package built successfully
Comment 19 Fedora Update System 2012-05-26 09:04:04 EDT
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
Comment 20 Fedora Update System 2012-05-26 13:52:53 EDT
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
Comment 21 Fedora Update System 2012-06-03 19:35:18 EDT
vdr-screenshot-0.0.14-4.fc16 has been pushed to the Fedora 16 stable repository.
Comment 22 Fedora Update System 2012-06-27 23:24:13 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.