Bug 548824 - Review Request: security-menus - Menu Structure for the Security Spin
Review Request: security-menus - Menu Structure for the Security Spin
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
12
All Linux
low Severity medium
: ---
: ---
Assigned To: Christoph Wickert
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-18 13:12 EST by Hiemanshu
Modified: 2014-09-30 08:45 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-02-17 13:57:27 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
cwickert: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hiemanshu 2009-12-18 13:12:57 EST
Spec URL: http://hiemanshu.fedorapeople.org/SPECS/security-menu.spec
SRPM URL: http://hiemanshu.fedorapeople.org/SRPMS/security-menu-1.0-1.fc12.src.rpm
Description:
This Package adds a Security Lab sub-menu to the 
xdg menu structure for GNOME and LXDE.
Comment 1 Christoph Wickert 2009-12-18 13:40:20 EST
This package still needs a lot of work, nevertheless I'm going to review it because I suggested something similar half a year ago.
Comment 2 Hiemanshu 2009-12-18 13:49:14 EST
I am fixing a couple of issues I missed, I will update the links when the new release is uploaded.
Comment 3 Hiemanshu 2009-12-18 13:52:50 EST
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
Comment 4 Hiemanshu 2009-12-18 14:06:53 EST
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
Comment 5 Hiemanshu 2009-12-19 07:06:30 EST
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
Comment 6 Christoph Wickert 2009-12-22 16:33:22 EST
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.
Comment 7 Christoph Wickert 2009-12-22 19:03:27 EST
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
Comment 8 Christoph Wickert 2010-01-06 11:13:01 EST
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.
Comment 9 Hiemanshu 2010-01-08 13:00:42 EST
I have been out on vacation, I will take care of these as soon as possible.

Regards,

Hiemanshu Sharma.
Comment 10 Hiemanshu 2010-01-12 06:33:34 EST
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.
Comment 11 Joerg (kital) Simon 2010-01-13 13:09:00 EST
(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
Comment 12 Hiemanshu 2010-01-13 15:09:47 EST
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.
Comment 13 Christoph Wickert 2010-01-14 17:51:51 EST
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.
Comment 14 Christoph Wickert 2010-01-14 19:51:18 EST
One more thing I forgot: We should rename the whole thing to "security-menus" (Plural), because all other packages are plural too.
Comment 15 Hiemanshu 2010-01-15 00:47:31 EST
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.
Comment 16 Christoph Wickert 2010-01-15 06:55:18 EST
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!
Comment 17 Christoph Wickert 2010-01-19 13:41:13 EST
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:
Comment 18 Jason Tibbitts 2010-01-19 14:21:44 EST
CVS done (by process-cvs-requests.py).
Comment 19 Christoph Wickert 2010-01-30 19:34:37 EST
Hiemanshu, can this be closed now?
Comment 20 Fabian Affolter 2014-09-30 08:22:59 EDT
Package Change Request
======================
Package Name: security-menus
New Branches: epel7 el6
Owners: fab
InitialCC:
Comment 21 Jon Ciesla 2014-09-30 08:45:27 EDT
Git done (by process-git-requests).

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