Bug 487737

Summary: Review Request: slock - Simple X display locker
Product: [Fedora] Fedora Reporter: Pavel Shevchuk <stlwrt>
Component: Package ReviewAssignee: Alexey Torkhov <atorkhov>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: atorkhov, fedora-package-review, notting
Target Milestone: ---Flags: atorkhov: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-03-27 16:13:19 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:
Attachments:
Description Flags
Patch for makefile to use rpm optflags none

Description Pavel Shevchuk 2009-02-27 17:55:42 UTC
Spec URL: http://rpm.scwlab.com/fedora/slock/slock.spec
SRPM URL: http://rpm.scwlab.com/fedora/slock/slock-0.9-1.fc10.src.rpm
Built RPMs for F10 i386 and x86_64: http://rpm.scwlab.com/fedora/slock/

Description:
slock is a simple X display locker. Really this is the simplest X screen locker
we are aware of. It is stable and quite a lot people in this community are
using it every day when they are out with friends or fetching some food from
the local pub.

Comment 1 Alexey Torkhov 2009-03-08 15:24:56 UTC
This package does not use standard rpm compiler flags when building:
https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags

Comment 2 Pavel Shevchuk 2009-03-08 16:08:45 UTC
Now it does.

* Sun Mar 08 2009 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9-2
- Set fedora generic compiler flags

Spec URL: http://rpm.scwlab.com/fedora/slock/slock.spec
SRPM URL: http://rpm.scwlab.com/fedora/slock/slock-0.9-2.fc10.src.rpm
Built RPMs for F10 i386 and x86_64: http://rpm.scwlab.com/fedora/slock/

Comment 3 Alexey Torkhov 2009-03-08 16:21:01 UTC
Generated debuginfo is empty. It happens because it have "-s" flag when running ld:
https://fedoraproject.org/wiki/Packaging/Debuginfo

I'm also suggesting to use patch, not sed.

Comment 4 Pavel Shevchuk 2009-03-15 17:57:10 UTC
* Sun Mar 15 2009 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9-3
- Fixed debuginfo generation

Spec URL: http://rpm.scwlab.com/fedora/slock/slock.spec
SRPM URL: http://rpm.scwlab.com/fedora/slock/slock-0.9-3.fc10.src.rpm
Built RPMs for F10 i386 and x86_64: http://rpm.scwlab.com/fedora/slock/

Comment 5 Alexey Torkhov 2009-03-15 21:08:39 UTC
- Replace two seds with a patch. It will be easier to read. Moreover, if upstream decide to change flags in makefile in future, sed will silently continue with old values and patch will fail.

- This is a GUI application, you should create .desktop file for it. See
https://fedoraproject.org/wiki/Packaging/Guidelines#desktop

Comment 6 Pavel Shevchuk 2009-03-15 21:13:52 UTC
This is NOT a gui application, it's expected to be ran by windowmanager, not user

Comment 7 Alexey Torkhov 2009-03-15 21:41:36 UTC
And there are standard way for window manager to choose locker? How it will know of slock existence?

Comment 8 Pavel Shevchuk 2009-03-15 23:13:36 UTC
Light WMs like openbox and dwm (slock was written for) bound commands, not fd.o .desktop nonsense.

How to create static patch for config.mk that will get arch-specific CFLAGS? I think sed expressions are pretty good options for one-line patching - my EFL/E17 specs have plenty of those.

Comment 9 Alexey Torkhov 2009-03-16 08:22:16 UTC
Created attachment 335317 [details]
Patch for makefile to use rpm optflags

(In reply to comment #8)
> Light WMs like openbox and dwm (slock was written for) bound commands,
> not fd.o .desktop nonsense.

Ok then. Please add a comment to spec telling that it is not need .desktop file and brief explanation.

> How to create static patch for config.mk that will get arch-specific CFLAGS?

Very simple - use $RPM_OPT_FLAGS environment variable. I've made a patch - it is in attachment as an example.

Programs with good makefiles do not require any patching at all - you can pass flags in make call just like "make CXXFLAGS=...", but this makefile written in bad way, so it'll not add includes to flags.

> I think sed expressions are pretty good options for one-line patching - my
> EFL/E17 specs have plenty of those.  

This is a bad practice in case of makefiles - you won't notice when upstream source changes. I think, sed is good in opposite cases - when patch would be ugly, or big, or when change made by sed if safe for changes, etc.

Comment 10 Pavel Shevchuk 2009-03-22 23:54:31 UTC
* Mon Mar 23 2009 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9-4
- Replaced sed expressions with a patch


Spec URL: http://rpm.scwlab.com/fedora/slock/slock.spec
SRPM URL: http://rpm.scwlab.com/fedora/slock/slock-0.9-4.fc10.src.rpm
Built RPMs for F10 i386 and x86_64: http://rpm.scwlab.com/fedora/slock/


Thank you for a patch, i didn't know $RPM_OPT_FLAGS is set in build environment

Comment 11 Alexey Torkhov 2009-03-23 20:13:41 UTC
+ rpmlint output:
slock.x86_64: E: setuid-binary /usr/bin/slock root 04755
slock.x86_64: E: non-standard-executable-perm /usr/bin/slock 04755
3 packages and 0 specfiles checked; 2 errors, 0 warnings.

Setuid binary is intended. Otherwise it can't check user password. It drops
privileges soon after start.

+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format
  %{name}.spec.
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The License field in the package spec file matches the actual license.
+ File, containing the text of the licenses for the package is included in
  %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source, as
  provided in the spec URL.
df342ad129cf2c3b8eb8da9d9d0ab845  slock-0.9.tar.gz
df342ad129cf2c3b8eb8da9d9d0ab845  slock-0.9.tar.gz

+ The package successfully compiles and builds into binary rpms on at least
  one primary architecture (x86_64).
+ All build dependencies are listed in BuildRequires.
+ No need to deal with locales.
+ Does not contain shared libraries.
+ The package does not designed to be relocatable.
+ A package owns all directories that it creates.
+ A package does list a file more than once in the spec %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
+ Does not contain large documentation files.
+ Includes only doc files in %doc.
+ No headers.
+ No static libraries.
+ The package does not contain pkgconfig(.pc) files.
+ The package does not contain library files with a suffix (e.g.
  libfoo.so.1.1).
+ No devel packages.
+ The package does not contain any .la libtool archives.
+ Does not need %{name}.desktop file.

This is not really GUI application and does not need desktop file. But please
add comment about this in spec before import.

+ The package does not own files or directories already owned by other
  packages.
+ At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
+ All filenames in the package are valid UTF-8.
+ The package builds in mock.
+ The package functions as described.

This package is APPROVED.

Comment 12 Pavel Shevchuk 2009-03-23 21:29:24 UTC
New Package CVS Request
=======================
Package Name: slock
Short Description: Simple X display locker
Owners: stalwart
Branches: F-10
InitialCC:

Comment 13 Kevin Fenzi 2009-03-24 17:16:30 UTC
cvs done.

Comment 14 Pavel Shevchuk 2009-03-27 16:13:19 UTC
Packages built, kudos to Alex and Kevin!