Bug 548824

This package still needs a lot of work, nevertheless I'm going to review it because I suggested something similar half a year ago.

I am fixing a couple of issues I missed, I will update the links when the new release is uploaded.

Spec URL is the same,
SRPM URL : http://hiemanshu.fedorapeople.org/SRPMS/security-menu-1.0-2.fc12.src.rpm

Link to koji scratch build :

http://koji.fedoraproject.org/koji/taskinfo?taskID=1880070

Updated, Fixed conflict with other packages
SRPM URL : http://hiemanshu.fedorapeople.org/SRPMS/security-menu-1.0-3.fc12.src.rpm

Link to Koji scratch build : 

http://koji.fedoraproject.org/koji/taskinfo?taskID=1880103

Final Release, Fixed all issues with package,
SPEC URL is the same,
SRPM : http://hiemanshu.fedorapeople.org/SRPMS/security-menu-1.0-4.fc12.src.rpm

Link to koji scratch build :
http://koji.fedoraproject.org/koji/taskinfo?taskID=1880993

Hi, I made a couple of changes to the source and checked them an at
https://fedorahosted.org/security-spin/browser/security-menu

Changes:
* Rename X-Security-Lab to X-SecurityLab for better fdo compliance. 
  Whitespces  are usually just left out.
* Add subcategories X-Anonymity, X-CodeAnalysis, X-Forensics,
  X-IntrusionDetection, X-Password-Tools, X-Reconnaissance and
  X-Wireless
* Use TryExec to determine installed apps, see
  http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s05.html
* Menu tweaks
* Enhance Makefile
* Added AUTHORS
* Added ChangeLog (for upstream changes, not packaging).
  Please review if my info is right!
  
Please take a look the changes, especially what I have done for the applications included with <filename>  </filename>. I think this might be needed for more apps, please check.

When we are done with everything, we tag it 1.0 in git make a tarball to package.

The spec looks sane, but
* you should use desktop-file-validate in %install, see
  https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
* Don't forget to include AUTHORS and ChangeLog
* Change the Source0 url to point to fedorahosted when we have a tarball there


Please don't upload the rpms into git, just put them on your fedorapeople.org account while they are under review.

Some more comments on the spec:

drop %define ..., it is not needed. If you need such things, use %global instead, see https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

Drop Requires(pre) and add
Requires:       redhat-menus
for directory ownership.

As long as we have no icons, remove the icon-cache scriptlets. When we have, add the latest version of the scripts again. Yours are slightly outdated compared to 
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

Can you please update the source package and make a release of it? Look at the changes I did in git, there are more packages that need to be fixed.

After that, please update the spec with the changes from the comments above, so I can review the package.

If you have any questions, don't hesitate to ask.

I have been out on vacation, I will take care of these as soon as possible.

Regards,

Hiemanshu Sharma.

I have updated with regards to your comments, Thank you for all the help.
Here are the updated files :

SPEC URL: http://hiemanshu.fedorapeople.org/SPECS/security-menu.spec
SRPM URL: http://hiemanshu.fedorapeople.org/SRPMS/security-menu-1.0-5.fc12.src.rpm

Link to koji scratch build : http://koji.fedoraproject.org/koji/taskinfo?taskID=1916324

Source0 is now pointed to : https://fedorahosted.org/released/security-spin/security-menu-1.0.tar.gz

If you need any more changes please let me know.

Hiemanshu Sharma.

(In reply to comment #10)
> If you need any more changes please let me know.

tested the rpm and i missed several submenus -

rebuild with patched security-lab.menu -

https://fedorahosted.org/security-spin/browser/security-lab.menu.patch

is working to make the missing submenus appear

I have updated with regards to your comments.
Here are the updated files :

SPEC URL: http://hiemanshu.fedorapeople.org/SPECS/security-menu.spec
SRPM URL:
http://hiemanshu.fedorapeople.org/SRPMS/security-menu-1.0-6.fc12.src.rpm

Link to koji scratch build :
http://koji.fedoraproject.org/koji/taskinfo?taskID=1919661

If you need any more changes please let me know.

Hiemanshu Sharma.

Review for 
1c3e5f77895ce44ba7a028d7a0f86d72  security-menu-1.0-6.fc12.src.rpm

OK - MUST: $ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/security-menu-1.0-6.fc13.*
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines: GPLv2
OK - MUST: License field in spec file matches the actual license: GPLv2
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
FIX - MUST: source doesn't match the upstream source by MD5
  from SRPM:        89de4bf505dab16f51a8779fc273a7d1
  from Source0 URL: 8d8113dc8009a6ddc002ba56ae18891b
OK - MUST: successfully compiles and builds into binary rpms on x86_64
OK - MUST: No ExcludeArch.
OK - MUST: all build dependencies are listed in BuildRequires.
N/A - MUST: handles locales properly with %find_lang
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review
OK - MUST: owns all directories that it creates (none)
OK - MUST: no duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: package has a %clean section, which contains rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ).
OK - MUST: consistently uses macros
OK - MUST: package contains code, or permissable content
N/A - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
N/A - MUST: Header files must be in a -devel package
N/A - MUST: Static libraries must be in a -static package
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix, then library files that end in .so must go in a -devel package.
N/A - MUST: devel packages must require the base package using a fully versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
OK - MUST: All desktop files are validated with desktop-file-install in the %install section.
OK - MUST: package does not own files or directories already owned by other packages.
OK - MUST: at the beginning of %install, the package runs rm -rf %{buildroot}
OK - MUST: all filenames valid UTF-8


SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: compiles and builds into binary rpms on all supported architectures.
OK - SHOULD: functions as described.
N/A - SHOULD: Scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Other items:
OK - latest stable version
OK - SourceURL valid


Please download the source again to make sure the md5 matches.

One more thing I forgot: We should rename the whole thing to "security-menus" (Plural), because all other packages are plural too.

I fixed the problem with the md5sum and changed the name as well. 

SPEC URL : http://hiemanshu.fedorapeople.org/SPECS/security-menus.spec
SRPM URL : http://hiemanshu.fedorapeople.org/SRPMS/security-menus-1.0-7.fc12.src.rpm
Koji Scratch Build : http://koji.fedoraproject.org/koji/taskinfo?taskID=1923035

Please let me know if anything else needs to be done.

Regards,

Hiemanshu Sharma.

renaming ok, md5sums ok, you are sponsored, everything is fine.

One final note: When downloading a file please use a download manager that preservers timestamps, because usually the timestamps in the srpm should be the same as the Source0 URL, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
I guess you first made the tarball and then uploaded it to fedorahosted, so it's not really important in this case.

The package is APPROVED. Good job!

Hiemanshu, you should request CVS, see
http://fedoraproject.org/wiki/CVS_admin_requests

Doing that for you now, so we cam move on


New Package CVS Request
=======================
Package Name: security-menus
Short Description: Menu Structure for the Security Spin
Owners: hiemanshu cwickert
Branches: F-11 F-12
InitialCC:

CVS done (by process-cvs-requests.py).

Hiemanshu, can this be closed now?

Package Change Request
======================
Package Name: security-menus
New Branches: epel7 el6
Owners: fab
InitialCC:

Git done (by process-git-requests).