Bug 248857

Summary: Review Request: schedtool - A tool to query or alter process scheduling policy
Product: [Fedora] Fedora Reporter: Adel Gadllah <adel.gadllah>
Component: Package ReviewAssignee: Thorsten Leemhuis <fedora>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, manuel.wolfshant, notting
Target Milestone: ---Flags: fedora: fedora-review+
wtogami: 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: 2007-08-12 08:25: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 Adel Gadllah 2007-07-19 09:51:55 UTC
Spec URL: http://tgmweb.at/gadllah/schedtool.spec
SRPM URL: http://tgmweb.at/gadllah/schedtool-1.2.9-1.fc7.src.rpm
Description: 
schedtool interfaces with the Linux CPU scheduler. It allows the user to set 
and query the CPU-affinity and nice-levels of processes, as well as all 
scheduling policies, like batch or real-time (RR/FIFO) classes and 
their priorities

Note: 
This is my first package so I need a sponsor.

Comment 1 manuel wolfshant 2007-07-19 10:52:13 UTC
There are two problems with your package
- please use the full URL
(http://freequaos.host.sk/schedtool/%{name}-%{version}.tar.bz2 for instance) in
the Source tag
- RPM_OPT_FLAGS are not used:

make CFLAGS="-Os -fomit-frame-pointer -s -pipe -DHAVE_AFFINITY" schedtool
make[1]: Entering directory `/builddir/build/BUILD/schedtool-1.2.9'
gcc -Os -fomit-frame-pointer -s -pipe -DHAVE_AFFINITY   -c -o schedtool.o
schedtool.c
schedtool.c: In function 'set_process':
schedtool.c:450: warning: pointer/integer type mismatch in conditional expression
gcc -Os -fomit-frame-pointer -s -pipe -DHAVE_AFFINITY   -c -o error.o error.c
gcc   schedtool.o error.o   -o schedtool
make[1]: Leaving directory `/builddir/build/BUILD/schedtool-1.2.9'


Comment 3 manuel wolfshant 2007-07-19 11:29:40 UTC
Quick glance at http://tgmweb.at/gadllah/schedtool-1.2.9-3.fc7.src.rpm

- the rpm_opt_flags problem is still there:
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.28294
+ umask 022
+ cd /builddir/build/BUILD
+ cd schedtool-1.2.9
+ LANG=C
+ export LANG
+ unset DISPLAY
+ make
make CFLAGS="-Os -fomit-frame-pointer -s -pipe -DHAVE_AFFINITY" schedtool
make[1]: Entering directory `/builddir/build/BUILD/schedtool-1.2.9'
gcc -Os -fomit-frame-pointer -s -pipe -DHAVE_AFFINITY   -c -o schedtool.o
schedtool.c
schedtool.c: In function 'set_process':
schedtool.c:450: warning: pointer/integer type mismatch in conditional expression
gcc -Os -fomit-frame-pointer -s -pipe -DHAVE_AFFINITY   -c -o error.o error.c
gcc   schedtool.o error.o   -o schedtool
make[1]: Leaving directory `/builddir/build/BUILD/schedtool-1.2.9'
+ exit 0
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.28294
+ umask 022
+ cd /builddir/build/BUILD
+ cd schedtool-1.2.9
+ LANG=C
+ export LANG
+ unset DISPLAY
+ rm -rf /var/tmp/schedtool-1.2.9-3.fc7-root-mockbuild
+ make install 'CFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic' D
ESTPREFIX=/usr DESTDIR=/var/tmp/schedtool-1.2.9-3.fc7-root-mockbuild
make CFLAGS="-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -DHAVE_AFFIN
ITY" schedtool
make[1]: Entering directory `/builddir/build/BUILD/schedtool-1.2.9'
make[1]: `schedtool' is up to date.

solution: you have to pass the RPM_OPT_FLAGS flag at build time, not at install

- the timestamp of the files is not preserved. you'd better revert to %doc and
remove the docdir line
- schedtool-debuginfo is empty


Comment 5 manuel wolfshant 2007-07-19 13:02:10 UTC
[Un]official package review (due to needsponsor status) coming soon

Comment 6 manuel wolfshant 2007-07-19 13:04:12 UTC
Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on:devel/x86_64
 [x] Rpmlint output: none
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging G
uidelines.
 [x] License field in the package spec file matches the actual license.
     License type:GPL v2
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(
s) for the package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     SHA1SUM of package: ab7743cba970d16ebe6ea8039e4bb57bd26027c8 
/home/wolfy/schedtool-1.2.9.tar.bz2
 [x] Package is not known to require ExcludeArch, OR:
     Arches excluded:
     Why:
 [-] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.
=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on:devel/x86_64 and F7/x86_64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:devel/x86_64 and F7/x86_64
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [-] File based requires are sane.


=== Issues ===
1. None

=== Final Notes ===
1. Everything seems OK


I would approve this package if the packager would not be in NEEDSPONSOR status.

Comment 7 Adel Gadllah 2007-07-21 06:28:48 UTC
new package and spec:
(new upstream release and dropped patch -> merged upstream)
http://tgmweb.at/gadllah/schedtool.spec
http://tgmweb.at/gadllah/schedtool-1.2.10-1.fc7.src.rpm



Comment 8 manuel wolfshant 2007-07-21 13:32:42 UTC
Package is still OK, builds fine in mock / F7 / x86_64.
Adel, I have a suggestion (it's pretty minor, feel free to ignore it): please
consider shipping the file CHANGES. I think it could be useful for some of the
users.

Comment 9 Adel Gadllah 2007-07-21 13:50:21 UTC
ok, thx for looking at the package again and for your suggestion.
I will ship the CHNAGES file too (nice to have and its not much work to include
it ;) )
Will upload a new one tomorrow morning or later today.

Comment 10 Adel Gadllah 2007-07-22 07:00:37 UTC
Ok, here is the new one (ship CHANGES too):
http://tgmweb.at/gadllah/schedtool.spec
http://tgmweb.at/gadllah/schedtool-1.2.10-2.fc7.src.rpm

Comment 11 Thorsten Leemhuis 2007-07-22 08:47:34 UTC
Blocker:
* Please s!%{_prefix}/bin/!%{_bindir}/! in %files section

Some other notes; please think about them and fix where you agree with them:

* the summary starts with "A " -- the rule of tumb iirc is to go without it
(e.g. Summary: Tool to foo)

* The description starts in lower case; rule of tumb iirc is to start capitalized

* Please tell upstream that

> Copyright (C) 19yy  <name of author>
> Gnomovision version 69, Copyright (C) 19yy name of author

in LICENSE looks bogus ;-)

* is there a specific reasons why you excluded TODO? I'd say it should be
shipped -- it's small and doesn't do any harm for those not intersted in it

* that DESTPREFIX stuff looks intersting, but well, it seems to be needed...

* might be better to not let the Makefile gzip the man page as rpm does this on
its own (in case rpm starts to use bz2 or whatever in the long term) 

Will approve the package and sponser you if you fix the blocker and comment on
the other stuff.

Comment 12 Adel Gadllah 2007-07-22 09:29:29 UTC
(In reply to comment #11)
> Blocker:
> * Please s!%{_prefix}/bin/!%{_bindir}/! in %files section
> 
ok fixed that one.

> Some other notes; please think about them and fix where you agree with them:
> 
> * the summary starts with "A " -- the rule of tumb iirc is to go without it
> (e.g. Summary: Tool to foo)
> 
ok, fixed

> * The description starts in lower case; rule of tumb iirc is to start capitalized
>
ok changed.
 
> * Please tell upstream that
> 
> > Copyright (C) 19yy  <name of author>
> > Gnomovision version 69, Copyright (C) 19yy name of author
> 
> in LICENSE looks bogus ;-)
> 
ok mail sent.

> * is there a specific reasons why you excluded TODO? I'd say it should be
> shipped -- it's small and doesn't do any harm for those not intersted in it
> 
ok shipped now.
> * that DESTPREFIX stuff looks intersting, but well, it seems to be needed...
> 
I already contacted upstream (and sent a patch) and they will fix it in the next
version.
> * might be better to not let the Makefile gzip the man page as rpm does this on
> its own (in case rpm starts to use bz2 or whatever in the long term) 
> 

this would require patching the makefile... is this really needed?
can add a patch to do it if its prefferd that way.

> Will approve the package and sponser you if you fix the blocker and comment on
> the other stuff.

ok, thx
here is the new spec and srpm:

http://tgmweb.at/gadllah/schedtool.spec
http://tgmweb.at/gadllah/schedtool-1.2.10-2.fc7.src.rpm



Comment 13 Adel Gadllah 2007-07-22 09:30:28 UTC
Correct URL is:
http://tgmweb.at/gadllah/schedtool-1.2.10-3.fc7.src.rpm
sorry for the typo

Comment 14 Thorsten Leemhuis 2007-07-22 09:44:54 UTC
(In reply to comment #12)
> > * might be better to not let the Makefile gzip the man page as rpm does this on
> > its own (in case rpm starts to use bz2 or whatever in the long term) 
> this would require patching the makefile... is this really needed?

No, likely not; I just wanted to make sure you know about it and keep it in mind.

> http://tgmweb.at/gadllah/schedtool-1.2.10-3.fc7.src.rpm

Approved.

Apply for cvs_extras in the accounts system and I'll sponsor you.

Comment 15 Adel Gadllah 2007-07-22 09:50:51 UTC
> Apply for cvs_extras in the accounts system and I'll sponsor you.

done.

Comment 16 Adel Gadllah 2007-07-22 10:37:11 UTC
New Package CVS Request
=======================
Package Name: schedtool
Short Description: Tool to query or alter process scheduling policy
Owners: adel.gadllah
Branches: FC-6 F-7