Bug 487737 - Review Request: slock - Simple X display locker
Review Request: slock - Simple X display locker
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alexey Torkhov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-27 12:55 EST by Pavel Shevchuk
Modified: 2009-03-27 12:13 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-27 12:13:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
atorkhov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch for makefile to use rpm optflags (574 bytes, patch)
2009-03-16 04:22 EDT, Alexey Torkhov
no flags Details | Diff

  None (edit)
Description Pavel Shevchuk 2009-02-27 12:55:42 EST
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 11:24:56 EDT
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 12:08:45 EDT
Now it does.

* Sun Mar 08 2009 Pavel "Stalwart" Shevchuk <stlwrt@gmail.com> - 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 12:21:01 EDT
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 13:57:10 EDT
* Sun Mar 15 2009 Pavel "Stalwart" Shevchuk <stlwrt@gmail.com> - 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 17:08:39 EDT
- 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 17:13:52 EDT
This is NOT a gui application, it's expected to be ran by windowmanager, not user
Comment 7 Alexey Torkhov 2009-03-15 17:41:36 EDT
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 19:13:36 EDT
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 04:22:16 EDT
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 19:54:31 EDT
* Mon Mar 23 2009 Pavel "Stalwart" Shevchuk <stlwrt@gmail.com> - 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 16:13:41 EDT
+ 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 17:29:24 EDT
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 13:16:30 EDT
cvs done.
Comment 14 Pavel Shevchuk 2009-03-27 12:13:19 EDT
Packages built, kudos to Alex and Kevin!

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