Bug 222594

Summary: Review Request: seedit: SELinux Policy Editor
Product: [Fedora] Fedora Reporter: Yuichi Nakamura <ynakam>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: mtasaka
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-01-31 14:03:23 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:
Bug Depends On:    
Bug Blocks: 163779    
Attachments:
Description Flags
Mock build log of on FC-devel i386
none
The output log on the execution of seedit-gui none

Description Yuichi Nakamura 2007-01-15 00:56:12 UTC
* Spec URL: 
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM URL: 
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.7.beta6.fc6.src.rpm

* Description: 
SELinux Policy Editor(SEEdit) is a tool to make SELinux easy.
It is composed of Simplified Policy, command line utils and GUI. 
The main feature is Simplified Policy. 
Simplified Policy is written in Simplified Policy Description Language(SPDL).
SPDL hides detail of SELinux.
For detail, visit http://seedit.sourceforge.net/.

rpmlint is silent in my environment.
It is my first package.
I need sponsor.

Comment 1 Yuichi Nakamura 2007-01-15 01:58:13 UTC
Sorry, please review following.
* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.7.beta6.src.rpm

Modified to use disttag to describe version number.


Comment 2 Xavier Lamien 2007-01-18 04:24:58 UTC
hi,

- for fisrt:
i checked out your spec file and...it doesn't meets with extras Packaging
Guidelines.
see http://fedoraproject.org/wiki/Packaging/Guidelines

Theres are a lot of things to review in your SPEC file,
a lot of things must be fix.

%{distro} can be remove, %{?dist} do the same thing.

#policy,gui subpackages are build only when arch is "noarch" --> why don't you
set them to noarch in spec file ?

your source0 must be point to the full path to verify the tarball.

you use cd and cd .. to enter and exit from an subfolder, you can use
pushd to enter and popd to exit.

your desktop-file-install command miss some option such as mode and -add-categories.

there is a lot of macros which's not in the right place.






Comment 3 Yuichi Nakamura 2007-01-18 07:18:09 UTC
(In reply to comment #2)

Thank you for comment.
> %{distro} can be remove, %{?dist} do the same thing.
Removed %{distro}.

>#policy,gui subpackages are build only when arch is "noarch" >--> why don't you
set them to noarch in spec file ?

I wanted to make noarch.rpm not i386.rpm for doc and gui subpackage, 
but it seems that such sub packages are usually build as i386.rpm in fedora. 
So I removed %if s related to noarch, it is more simple.

>your source0 must be point to the full path to verify the tarball.
Fixed.

>you use cd and cd .. to enter and exit from an subfolder, you can use pushd to
enter and popd to exit.
Fixed.

> your desktop-file-install command miss some option such as mode and
-add-categories.
Fixed, add "-add-categories".

> there is a lot of macros which's not in the right place.
I looked at macros section of package guide line, 
but I am not sure what you mean.
Can you tell me one example?

Other fixes: 
- Added forgotten BuildRequires for libselinux,libsepol.
- Added %{?_smp_mflags} to make

New SPEC and SRPM:
* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.8.beta6.1.src.rpm


Comment 4 Ralf Corsepius 2007-01-18 08:13:41 UTC
(In reply to comment #3)
> (In reply to comment #2)
>
> >#policy,gui subpackages are build only when arch is "noarch" >--> why don't you
> set them to noarch in spec file ?
> 
> I wanted to make noarch.rpm not i386.rpm for doc and gui subpackage, 
> but it seems that such sub packages are usually build as i386.rpm in fedora.
Right, this is impossible. All packages being built from a common src.rpm share
the same arch.

More comments:

- MUSTFIX: Package doesn't honor RPM_OPT_FLAGS:
...
gcc -c -Wall -O2 -g -DDEBUG=1 -I../include  -I/usr/include/selinux lex.yy.c
..

- SHOULDFIX: Incorrect include directory -I/usr/include
...
cc -Werror -Wall -W -I/usr/include -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE
-I../include -I/usr/include/selinux -Dfc6   -c -o seedit-restorecon.o 
...
Passing -I/usr/include is a mistake. You should never pass -I/usr/include.


- MUSTFIX: Directory /usr/lib/python2.4/site-packages/seedit is unowned.
The seedit package must known this directory.



Comment 5 Ralf Corsepius 2007-01-18 08:14:29 UTC
(In reply to comment #4)

> - MUSTFIX: Directory /usr/lib/python2.4/site-packages/seedit is unowned.
> The seedit package must known this directory.
s/known/own/




Comment 6 Xavier Lamien 2007-01-18 12:26:49 UTC
(In reply to comment #4)

> - MUSTFIX: Directory /usr/lib/python2.4/site-packages/seedit is unowned.
> The seedit package must known this directory.
s/known/own/

more info:
your directory /usr/lib/python2.4/site-packages/seedit
should look like this --> %{python_sitelib}/%{name}/
where your defined python_sitelib must set above with something like this:
%define python_sitelib %(%{__python} -c 'from distutils import sysconfig; print
sysconfig.get_python_lib()')


> >#policy,gui subpackages are build only when arch is "noarch" >--> why don't you
> set them to noarch in spec file ?
> 
> I wanted to make noarch.rpm not i386.rpm for doc and gui subpackage, 
> but it seems that such sub packages are usually build as i386.rpm in fedora.
>Right, this is impossible. All packages being built from a common src.rpm share
>the same arch.

i meant to set subpakage section to "buildarch:     noarch"

more comment:

vendor option in desktop-file-install should set to vendor ""
set it to fedora is not required any more.

in %post policy:
touch %{_datadir}/share/seedit/sepolicy/need-init 
--> %{_datadir}/%{name}/sepolicy/need-init
same thing for the other below.

%doc should be place just after %defattr macros and on the same line.

missing argument:
%files -f %{name}.lang
see Handling Locale Files section in
http://fedoraproject.org/wiki/Packaging/Guidelines



Comment 7 Yuichi Nakamura 2007-01-19 03:36:05 UTC
(In reply to comment #5)
>- MUSTFIX: Package doesn't honor RPM_OPT_FLAGS:
>...
>gcc -c -Wall -O2 -g -DDEBUG=1 -I../include  -I/usr/include/selinux lex.yy.c
>..
Fixed. Modified spec file and Makefiles.

>- SHOULDFIX: Incorrect include directory -I/usr/include
>...
>cc -Werror -Wall -W -I/usr/include -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE
>-I../include -I/usr/include/selinux -Dfc6   -c -o seedit-restorecon.o 
>...
>Passing -I/usr/include is a mistake. You should never pass -I/usr/include.
Fixed.

>- MUSTFIX: Directory /usr/lib/python2.4/site-packages/seedit is unowned.
>The seedit package must own this directory.
Fixed.

Comment 8 Yuichi Nakamura 2007-01-19 03:36:57 UTC
(In reply to comment #6)
>more info:
>your directory /usr/lib/python2.4/site-packages/seedit
>should look like this --> %{python_sitelib}/%{name}/
>where your defined python_sitelib must set above with something like this:
>%define python_sitelib %(%{__python} -c 'from distutils import sysconfig; print
>sysconfig.get_python_lib()')
It is very useful, thanks!
Fixed.

>vendor option in desktop-file-install should set to vendor ""
>set it to fedora is not required any more.
Fixed.

>in %post policy:
>touch %{_datadir}/share/seedit/sepolicy/need-init 
>--> %{_datadir}/%{name}/sepolicy/need-init
>same thing for the other below.
Fixed.

>%doc should be place just after %defattr macros and on the same line.
Fixed.

>missing argument:
>%files -f %{name}.lang
>see Handling Locale Files section in
>http://fedoraproject.org/wiki/Packaging/Guidelines
Fixed, but there is no locale file in seedit package.
locale file exists only in gui subpackage.




Comment 9 Yuichi Nakamura 2007-01-19 03:38:46 UTC
Current spec file and src.rpm file is following:

* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.9.beta6.2.src.rpm

Comment 10 Mamoru TASAKA 2007-01-23 07:02:46 UTC
Created attachment 146272 [details]
Mock build log of  on FC-devel i386

Well,
* BuildRequires, etc
  - Firstly, I cannot check this package because mockbuild
  fails on FC-devel i386. Please check the attached
  log.

* File/Directory entries
  - For main package, file entries include:
---------------------------------------------
%{python_sitelib}/%{name}
%{_datadir}/%{name}
---------------------------------------------
    And, for example -policy subpackage include:
---------------------------------------------
%{_datadir}/%{name}/scripts/seedit-installhelper.sh
----------------------------------------------
    ... Then seedit-installhelper.sh is installed in
    both main and -policy subpackage, because
----------------------------------------------
%{_datadir}/%{name}
----------------------------------------------
    is interpretted as the directory %{_datadir}/%{name}
    itself _and_ all the files/directories etc under
    %{_datadir}/%{name}.
    If you want to indicate only the directory, you have
    to write:
----------------------------------------------
%dir %{_datadir}/%{name}
----------------------------------------------
    Please remove duplicate entries, then ensure that
    every directories created during the install of
    seedit related packages are owned by one package.

* Requires
-----------------------------------------------
Requires: python >= 2.3
----------------------------------------------- 
    This is not needed. Python version dependency is
    automatically added by rpmbuild process.

* %build process, etc...
-----------------------------------------------
--add-category X-Fedora
-----------------------------------------------
    The category "X-???" is deprecated and should be
    removed.
------------------------------------------------
install -m 664 %{SOURCE2}
------------------------------------------------
     Why should this be 0664, not 0644?

* scriptlets:
  ... By the way, what if user sets selinux policy as
  "DISABLED"? There may be the case in which sysadmin
  has to disable selinux for some reason. In the case,
  selinux degree is "upgraded" to TARGETED?

* For header description:
------------------------------------------------------
Source0:
http://prdownloads.sourceforge.jp/selpe/23577/%{name}-%{version}-%{betatag}.tar.gz

------------------------------------------------------
  Please change the URL of Source0 so that I can directly
  gain the source by "wget -N .........".
  Executing "wget -N" against this URL only pulls a HTML
  file.

Comment 11 Mamoru TASAKA 2007-01-23 07:19:30 UTC
(In reply to comment #10)
> * scriptlets:
>   ... By the way, what if user sets selinux policy as
>   "DISABLED"? There may be the case in which sysadmin
>   has to disable selinux for some reason. In the case,
>   selinux degree is "upgraded" to TARGETED?

Oops, s|TARGETED|permissive|...
Then: anyway should selinux degree be set as permissive when
removing this anyway? If selinux policy is set correctly by
this application (seedit), the selinux policy should work
even after this application is removed, shouldn't it?

Comment 12 Yuichi Nakamura 2007-01-23 08:12:10 UTC
(In reply to comment #10)

Mamoru, thanks for comments.

>Well,
* BuildRequires, etc
>  - Firstly, I cannot check this package because mockbuild
>  fails on FC-devel i386. Please check the attached
>  log.
Fixed.
Added flex and byacc in buildrequires.

> * File/Directory entries
>  - For main package, file entries include:
>    Please remove duplicate entries, then ensure that
>    every directories created during the install of
>    seedit related packages are owned by one package.
Fixed.


>* Requires
>-----------------------------------------------
>Requires: python >= 2.3
>----------------------------------------------- 
>    This is not needed. Python version dependency is
>    automatically added by rpmbuild process.
Fixed.

>* %build process, etc...
>-----------------------------------------------
>--add-category X-Fedora
>-----------------------------------------------
>    The category "X-???" is deprecated and should be
>    removed.
Fixed.

>------------------------------------------------
>install -m 664 %{SOURCE2}
>------------------------------------------------
>     Why should this be 0664, not 0644?
It was only a mis-spell.
Fixed.

>* For header description:
>  Please change the URL of Source0 so that I can directly
>  gain the source by "wget -N .........".
>  Executing "wget -N" against this URL only pulls a HTML
>  file.
Fixed.


>* scriptlets:
>  ... By the way, what if user sets selinux policy as
>  "DISABLED"? There may be the case in which sysadmin
>  has to disable selinux for some reason. In the case,
>  selinux degree is "upgraded" to TARGETED?
Hmm, I will reply in next comment.

Comment 13 Yuichi Nakamura 2007-01-23 08:14:22 UTC
SELinux Policy Editor generates its own policy under /etc/selinux/seedit.
It can not be editted without seedit, seedit-policy packages.
And when user uses seedit for the first time, 
/etc/selinux/seedit is overwritten.
SELINUXTYPE=targeted
->
SELINUXTYPE=seedit

So, when seedit is uninstalled, 
/etc/selinux/seedit becomes unuseful, because it can not be editted.
So /etc/selinux/config should be editted.
SELINUXTYPE=seedit
->
SELINUXTYPE=targeted

>* scriptlets:
>  ... By the way, what if user sets selinux policy as
>  "DISABLED"? There may be the case in which sysadmin
>  has to disable selinux for some reason. In the case,
>  selinux degree is "upgraded" to TARGETED?
I've understood what you say, and I agree.
In the latest seedit.spec,
it will not touch "SELINUX=" line.

Comment 14 Yuichi Nakamura 2007-01-23 08:15:26 UTC
Current spec file and src.rpm file is following:

* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.10.beta6.2.src.rpm

Comment 15 Mamoru TASAKA 2007-01-23 12:53:01 UTC
Well, I
* install 0.10 -gui package
* changed selinux : "disabled" -> "permissive"
* reboot
* try "seedit-gui"
Then...
-------------------------------------------------
[tasaka1@localhost ~]$ seedit-gui 
sh: /usr/share/seedit/scripts/seedit-installhelper.sh: そのようなファイルやディ
レクトリはありません
-------------------------------------------------
(for non-Japanese users: "そのようなファイル..." means
 "no such file or directory")

What is happening?

Comment 16 Yuichi Nakamura 2007-01-23 14:21:28 UTC
OOps!
It is due to resent changes about paths.
I have forgotten test seedit itself..

I will fix it tomorrow,


Comment 17 Yuichi Nakamura 2007-01-24 06:54:08 UTC
(In reply to comment #16)
Following is latest version:
I installed rpms and they worked..

* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.11.beta6.3.src.rpm



Comment 18 Mamoru TASAKA 2007-01-24 11:32:24 UTC
Created attachment 146400 [details]
The output log on the execution of seedit-gui

Well, now:
* On the first time I executed seedit-gui, I was
  asked to reboot the system, so did I.
* Then on the second time I executed, I got the
  error attached.
* On the third time, it seems that seedit-gui works
  well.
Is this the expected behavior?

For packaging issue, I will check later.

Comment 19 Yuichi Nakamura 2007-01-25 02:05:16 UTC
(In reply to comment #18)
It is unexpected behavior, thanks for report.
It is a bug when upgrading package, and I have fixed now.

And please input 
/usr/sbin/seedit-rbac off -n
before upgrade.(it is needed only this time, will not needed next time.)

Fixed version..
* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.12.beta6.4.src.rpm


Comment 20 Xavier Lamien 2007-01-25 02:56:25 UTC
>And please input 
>/usr/sbin/seedit-rbac off -n
>before upgrade.(it is needed only this time, will not needed next time.)

I think you should past this in your updated spec file for updated release.


Comment 21 Yuichi Nakamura 2007-01-25 04:36:09 UTC
(In reply to comment #20)
> >And please input 
> >/usr/sbin/seedit-rbac off -n
> >before upgrade.(it is needed only this time, will not needed next time.)
> I think you should past this in your updated spec file for updated release.
I see, it is more kind. 
I fixed seedit itself.
"/usr/sbin/seedit-rbac off -n" is no longer necessary.

Latest version..
* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.13.beta6.5.src.rpm




Comment 22 Mamoru TASAKA 2007-01-25 18:17:29 UTC
Well, for packaing issue:

* Requires:
  - gnome-python2 (required by -gui)
    Please check if this is really required. By checking with
    "grep import" no module seems to be installed from
    gnome-python2.

* File ownership issue/scriptlet
  - - policy package:
-------------------------------------------------------
if [ $1 = 2 ]; then
        #Mark to initialize RBAC config when upgrade
        touch /usr/share/seedit/sepolicy/need-rbac-init
fi
-------------------------------------------------------
    However:
-------------------------------------------------------
[root@localhost ~]# touch /usr/share/seedit/sepolicy/need-rbac-init
[root@localhost ~]# LANG=C rpm -qf /usr/share/seedit/sepolicy/need-rbac-init
file /usr/share/seedit/sepolicy/need-rbac-init is not owned by any package
-------------------------------------------------------
    This file (and perhaps also /usr/share/seedit/sepolicy/need-init)
    should be marked as %ghost file

    And please check if any other file which should be marked
    as such exists so that all _unnessary_ files are correctly
    removed on the complete removal of seedit.

* Version dependency requirement
-------------------------------------------------------
Requires: seedit >= 2.1.0
-------------------------------------------------------
  - Usually these types of requirement should be version-release
    dependent, i.e.
-------------------------------------------------------
Requires: %{name} = %{version}-%{release}
-------------------------------------------------------

* Desktop file:
-------------------------------------------------------
Categories=Application;SystemSetup;X-Red-Hat-Base;
-------------------------------------------------------
  Both categories: "Application" "X-Red-Hat-Base" "SystemSetup"
  are now deprecated and these should be removed.

  From desktop-file-validate:
-------------------------------------------------------
Categories values must be one of 
"AudioVideo", "Audio", "Video", "Development", "Education", 
"Game", "Graphics", "Network", "Office", "Settings", 
"System", "Utility", "Building", "Debugger", "IDE", 
"GUIDesigner", "Profiling", "RevisionControl", "Translation", 
"Calendar", "ContactManagement", "Database", "Dictionary",
"Chart", "Email", "Finance", "FlowChart", "PDA", 
"ProjectManagement", "Presentation", "Spreadsheet", "WordProcessor",
"2DGraphics", "VectorGraphics", "RasterGraphics", "3DGraphics", 
"Scanning", "OCR", "Photography", "Viewer", "DesktopSettings", 
"HardwareSettings", "PackageManager", "Dialup", "InstantMessaging", 
"IRCClient", "FileTransfer", "HamRadio", "News", "P2P", 
"RemoteAccess", "Telephony", "WebBrowser", "WebDevelopment", 
"Midi", "Mixer", "Sequencer", "Tuner", "TV", "AudioVideoEditing", 
"Player", "Recorder", "DiscBurning", "ActionGame", "AdventureGame", 
"ArcadeGame", "BoardGame", "BlocksGame", "CardGame", "KidsGame", 
"LogicGame", "RolePlaying", "Simulation", "SportsGame", 
"StrategyGame", "Art", "Construction", "Music", "Languages", 
"Science", "Astronomy", "Biology", "Chemistry", "Geology", "Math", 
"MedicalSoftware", "Physics", "Amusement", "Archiving", 
"Electronics", "Emulator", "Engineering", "FileManager", 
"TerminalEmulator", "Filesystem", "Monitor", "Security", 
"Accessibility", "Calculator", "Clock", "TextEditor", "Core", 
"KDE", "GNOME", "GTK", "Qt", "Motif", "Java", "ConsoleOnly", 
"Screensaver", "TrayIcon", "Applet", "Shell"
-------------------------------------------------------

* Timestamps
  - These packages include many text files, image files
    and keeping timestamps on these files are generally 
    preferred. Please fix so that the timestamps on these
    files are kept.

-------------------------------------------------------
install -m 0644 %{SOURCE2} ${RPM_BUILD_ROOT}%{_datadir}/pixmaps/seedit-gui.png
-------------------------------------------------------
    Also, please use "install -p".

* Macros
-------------------------------------------------------
%define selinuxconf /etc/selinux/config
%define auditrules /etc/audit/audit.rules
-------------------------------------------------------
  Please check if the directory /etc should be written
  as hardcoded or as %{_sysconfdir}.

* Pam requirement
  %{_sysconfdir}/pam.d/seedit-gui includes:
-------------------------------------------------------
auth            include         config-util
-------------------------------------------------------
  This sentence requires pam >= 0.80 so I think
  adding "Requires: pam >= 0.80" is preferable.

* $RPM_BUILD_ROOT vs %{buildroot}
  Please use one, not both.

Comment 23 Mamoru TASAKA 2007-01-26 03:18:47 UTC
And.. config-util (/etc/pam.d/config-util) is perhaps
Fedora/Redhat specific and requires pam >= 0.80-9.


Comment 24 Yuichi Nakamura 2007-01-26 04:36:34 UTC
Thank you for review!

>* Requires:
>  - gnome-python2 (required by -gui)
>    Please check if this is really required. By checking with
>    "grep import" no module seems to be installed from
>    gnome-python2.
Fixed, I am not using it now.

>* File ownership issue/scriptlet
>    And please check if any other file which should be marked
>    as such exists so that all _unnessary_ files are correctly
>    removed on the complete removal of seedit.
Fixed, I listed rbac-init, need-init in %ghost.

>* Version dependency requirement
>-------------------------------------------------------
>Requires: seedit >= 2.1.0
>-------------------------------------------------------
>  - Usually these types of requirement should be version->release
>    dependent, i.e.
>-------------------------------------------------------
>Requires: %{name} = %{version}-%{release}
>-------------------------------------------------------
Fixed.
>Requires: %{name} = %{version}-%{release}
did not work("=" does not work), 
so I used 
Requires: %{name} >= %{version}-%{release}


>* Desktop file:
>-------------------------------------------------------
>Categories=Application;SystemSetup;X-Red-Hat-Base;
>-------------------------------------------------------
>  Both categories: "Application" "X-Red-Hat-Base" "SystemSetup"
>  are now deprecated and these should be removed.
Fixed.

>* Timestamps
>  - These packages include many text files, image files
>    and keeping timestamps on these files are generally 
>    preferred. Please fix so that the timestamps on these
>    files are kept.
>
>-------------------------------------------------------
>install -m 0644 %{SOURCE2} ${RPM_BUILD_ROOT}%{_datadir}/pixmaps/seedit-gui.png
>-------------------------------------------------------
>    Also, please use "install -p".
Fixed both spec file and Makefiles.

>* Macros
>-------------------------------------------------------
>%define selinuxconf /etc/selinux/config
>%define auditrules /etc/audit/audit.rules
>-------------------------------------------------------
>  Please check if the directory /etc should be written
>  as hardcoded or as %{_sysconfdir}.
Fixed.

>* Pam requirement
>  %{_sysconfdir}/pam.d/seedit-gui includes:
>-------------------------------------------------------
>auth            include         config-util
>-------------------------------------------------------
>  This sentence requires pam >= 0.80 so I think
>  adding "Requires: pam >= 0.80" is preferable.
Yes, in old pam, it does not work.
Fixed.

>* $RPM_BUILD_ROOT vs %{buildroot}
>  Please use one, not both.
Fixed. Using only %{buildroot}

Updated version:
* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.14.beta6.6.src.rpm

Comment 25 Mamoru TASAKA 2007-01-26 09:20:42 UTC
For -0.14:

(In reply to comment #24)
> >-------------------------------------------------------
> >  - Usually these types of requirement should be version->release
> >    dependent, i.e.
> >-------------------------------------------------------
> >Requires: %{name} = %{version}-%{release}
> >-------------------------------------------------------
> Fixed.
> >Requires: %{name} = %{version}-%{release}
> did not work("=" does not work), 
> so I used 
> Requires: %{name} >= %{version}-%{release}
What do you mean? Why equality is not valid for this package?
Theoretically, inequality admits the different version between
main package and its subpackages.

> Fixed.
Okay. Now I can correctly see seedit-gui entry.
> 
> >* Timestamps
> >  - These packages include many text files, image files
> >    and keeping timestamps on these files are generally 
> >    preferred. Please fix so that the timestamps on these
> >    files are kept.
> Fixed both spec file and Makefiles.
Still some files are installed by "install -m 755" or "cp -r",
not "install -p -m 755" or "cp -pr"

> >-------------------------------------------------------
> >auth            include         config-util
> >-------------------------------------------------------
> >  This sentence requires pam >= 0.80 so I think
> >  adding "Requires: pam >= 0.80" is preferable.
> Yes, in old pam, it does not work.
> Fixed.
For this, I think "pam >= 0.80-9" is preferable as said
in comment 23, because config-util is introduced on 0.80-9.

Nevertheless, the left issues are rather small. So:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review request and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/Extras/HowToGetSponsored


Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora Extras package review requests which are waiting for someone to review
can be checked on:
https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=FE-NEW&hide_resolved=1

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 26 Xavier Lamien 2007-01-26 16:19:06 UTC
> >  This sentence requires pam >= 0.80 so I think
> >  adding "Requires: pam >= 0.80" is preferable.
> Yes, in old pam, it does not work.
> Fixed.
>For this, I think "pam >= 0.80-9" is preferable as said
>in comment 23, because config-util is introduced on 0.80-9.

I think that PAM is not really obligatory as requires, considering it is the
default authentication of fedora system, it's already installed from first
installation of fedora Core.
On the other hand, it would surely be necessary to add "usermode" in fields
requires.


Comment 27 Mamoru TASAKA 2007-01-26 16:26:01 UTC
(In reply to comment #26)

> I think that PAM is not really obligatory as requires, 
What I mention is this requires pam ">= 0.80-9", not only pam,
because "include config-util" sentence cannot be interpretted
under pam 0.80-8.

Comment 28 Yuichi Nakamura 2007-01-27 01:24:36 UTC
(In reply to comment #25)
> > >Requires: %{name} = %{version}-%{release}
> > did not work("=" does not work), 
> > so I used 
> > Requires: %{name} >= %{version}-%{release}
> What do you mean? Why equality is not valid for this package?
> Theoretically, inequality admits the different version between
> main package and its subpackages.
Fixed. It works now.
It seems that I misspelled something :-)


> Still some files are installed by "install -m 755" or "cp -r",
> not "install -p -m 755" or "cp -pr"
OOps, I've greped build log, and removed cp -r, install -m.

> For this, I think "pam >= 0.80-9" is preferable as said
> in comment 23, because config-util is introduced on 0.80-9.
Fixed, 
About usermode, I've already have it in Requres field.


Comment 29 Yuichi Nakamura 2007-01-27 01:29:23 UTC
(In reply to comment #25)

Updated version:
* SPEC
http://prdownloads.sourceforge.jp/selpe/23577/seedit.spec
* SRPM
http://prdownloads.sourceforge.jp/selpe/23577/seedit-2.1.0-0.15.beta6.7.src.rpm

> -------------------------------------------------------------
> NOTE: Before being sponsored:
<snip>
> Usually there are two ways to show this.
> A. submit other review requests with enough quality.
> B. Do a "pre-review" of other person's review request
>    (at the time you are not sponsored, you cannot do
>    a formal review)
I see.
I have no more package to submit, so I will take "B".
It might take time, but I will do some pre-review.
Please wait and see my pre-review.



Comment 30 Yuichi Nakamura 2007-01-30 05:48:27 UTC
I have read guidelines again and did two pre-review today..
Bug 225119 and Bug 222338 .


Comment 31 Mamoru TASAKA 2007-01-30 12:11:05 UTC
Well,

* This package (seedit) is good.
* md5sum of source coincides.
* Your pre-review looks good to a certain content.
- Note:
  My recognition is that setting the id of vendor as 
  "fedora" is still recommended if upstream does not
  set vendor id.
  http://fedoraproject.org/wiki/Packaging/Guidelines

Okay!!
------------------------------------------------------------
  This package (seedit) is APPROVED by me.
------------------------------------------------------------

  Please go forward according to 
  http://fedoraproject.org/wiki/Extras/Contributors
  I will sponsor you when you do a procedure and I receive
  a mail which tells that you need a sponsor.

Comment 32 Yuichi Nakamura 2007-01-31 04:32:49 UTC
I am now sponsored, thank you very much!
I will import seedit this midnight.


Comment 33 Yuichi Nakamura 2007-01-31 13:51:21 UTC
I've imported seedit now, and built successfully.

Mamoru, I really appreciate reviewing and sponsoring me.
To all, thanks to very useful comments !
I will close this bug.


Comment 34 Mamoru TASAKA 2007-02-13 04:33:07 UTC
Yuichi,

Well I saw you upgraded seedit to 2.1.0, however current
seedit has
----------------------------------------------
ynakam AT hitachisoft.jp:
    seedit
      FE6 > FE7 (0:2.1.0-3.fc6 > 0:2.1.0-2.fc7)
----------------------------------------------
So please bump the release of FE7 seedit to -3
for now. 
(Note: this mail should be received if you are 
subscribing to fedora-maintainers mailing list).

Note: when you failed to send a queue by some mistakes (e.g.
just forgot to add a patch, making a tag failed with some
reason..) and you just want to bump release, you can set the
release as 2%{?dist}.1, for example. 

The details are:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#DistBump

Comment 35 Yuichi Nakamura 2007-02-13 08:29:22 UTC
Thanks, fixed it now.