Bug 249522

Hi, I am not a sponsor, but I saw you spec file.

1)
>Buildrequires: checkpolicy libselinux-devel >= 2.0.13 selinux-policy-devel =
2.6.4-26.sepgsql.fc7
>Requires: policycoreutils >= 2.0.16 libselinux >= 2.0.13 selinux-policy =
2.6.4-26.sepgsql.fc7
I could not build your package.
What is selinux-policy-devel = "2.6.4-26.sepgsql.fc7"?
It it included in fedora?

2)
You have to use more macros throughout spec file.
Such as:
sepostgresql -> %{name}
/usr/share/selinux/devel/Makefile -> %{__datadir}/selinux/devel/Makefile
and also /usr/sbin etc, you can replace them with macros.
See: http://fedoraproject.org/wiki/Packaging/RPMMacros

3)
Your package does not include documentation.
http://fedoraproject.org/wiki/Packaging/Guidelines#head-9bbfa57478f0460c6160947a6bf795249488182b
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
> - MUST: 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 must be included in %doc.

4)
Are you using $RPM_OPT_FLAGS/%{optflags}?
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448

One more comment,
I can not find download url for source files.

This may be useful:
http://fedoraproject.org/wiki/Packaging/SourceURL

Thanks for your reviewing. The packages are updated based on them.
http://code.google.com/p/sepgsql/downloads/list
Spec URL: http://sepgsql.googlecode.com/files/sepostgresql.spec
SRPM URL: http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-
0.407.beta.fc8.src.rpm

> (1).
> I could not build your package.
> What is selinux-policy-devel = "2.6.4-26.sepgsql.fc7"?
> It it included in fedora?
It is a modified selinux-policy package.
Several Definitions for database related object classes and access vectores are 
added. Because these definitons are currently included in the default selinux-
policy package, we also have to submit the additional definitions into the 
default selinux-policy package.

You can obtain the additional definitions from here:
http://sepgsql.googlecode.com/svn/policy/refpolicy-add-sepgsql-
definitions.fedora8.patch

> (2)
> You have to use more macros throughout spec file.
> Such as:
> sepostgresql -> %{name}
> /usr/share/selinux/devel/Makefile -> %{__datadir}/selinux/devel/Makefile
> and also /usr/sbin etc, you can replace them with macros.

Several standard paths are replaced by the macros.
/etc/rc.d/init.d -> %{_initrddir}
/usr/share -> %{_datadir}
However, I kept a part using /usr/sbin as a path of commands, like semodule,
because these are not changed when %{_prefix} of sepostgresql is over-written.
In addition, this manner follows the "Packaging SELinux Policy Modules (draft)".
  http://fedoraproject.org/wiki/PackagingDrafts/SELinux/PolicyModules

> (3)
> Your package does not include documentation.
The following line was added.
  %doc COPYRIGHT README HISTORY

SE-PostgreSQL is distributed under BSD license, as PostgreSQL.

> (4)
> I can not find download url for source files.

"Source0" got a full location to indicate full location, as follows:
Source0: ftp://ftp.postgresql.org/pub/source/v%{version}/postgresql-%
{version}.tar.gz

Thanks,

Oops, some fix in my comments:

OLD:(1)  Because these definitons are currently included in ...
NEW:(1)' Because these definitons are NOT currently included in ...

OLD:(4)  "Source0" got a full location to indicate full location, as follows:
NEW:(4)' "Source0" got a full location to indicate, as follows:

(In reply to comment #3)

Created attachment 160476 [details]
spec file patch

- removes a duplicate Group entry
- changes the buildroot to comply with fedora packaging guidelines
- uses the useradd scriptlet recipe from:
  http://fedoraproject.org/wiki/PackagingDrafts/UsersAndGroups
- does not delete users and groups in postun, per the same guidelines

did you successfully build this in mock?  there seem to be missing buildrequires
when i attempt it.

current rpmlint output:
E: sepostgresql non-standard-uid /var/lib/sepgsql sepgsql
E: sepostgresql non-standard-gid /var/lib/sepgsql sepgsql
E: sepostgresql non-standard-dir-perm /var/lib/sepgsql 0700
E: sepostgresql non-standard-uid /var/lib/sepgsql/data sepgsql
E: sepostgresql non-standard-gid /var/lib/sepgsql/data sepgsql
E: sepostgresql non-standard-dir-perm /var/lib/sepgsql/data 0700
E: sepostgresql non-standard-uid /var/lib/sepgsql/backups sepgsql
E: sepostgresql non-standard-gid /var/lib/sepgsql/backups sepgsql
E: sepostgresql non-standard-dir-perm /var/lib/sepgsql/backups 0700
W: sepostgresql incoherent-version-in-changelog 8.2.4-0.407 8.2.4-0.407.beta.fc8
W: sepostgresql dangerous-command-in-%postun userdel
E: sepostgresql subsys-not-used /etc/rc.d/init.d/sepostgresql

Thanks for your comments.

(In reply to comment #5)
> Created an attachment (id=160476) [edit]
> spec file patch
> - removes a duplicate Group entry
> - changes the buildroot to comply with fedora packaging guidelines
> - uses the useradd scriptlet recipe from:
>   http://fedoraproject.org/wiki/PackagingDrafts/UsersAndGroups
I applied these fixes.

> - does not delete users and groups in postun, per the same guidelines
I commented out the userdel and groupdel commands, not removed, because the 
postgresql-server package contains the same commands.
I want to keep it for a while, if no matter.

(In reply to comment #6)
> did you successfully build this in mock?  there seem to be missing 
buildrequires
> when i attempt it.

Your patch intended to add "Requires(pre): shadow-utils", but it is not 
necessary because we can assume some fundamental packages are installed.
See, http://fedoraproject.org/wiki/Packaging/FullExceptionList
The shadow-utils is also contained the list.

> current rpmlint output:
> E: sepostgresql non-standard-uid /var/lib/sepgsql sepgsql
> E: sepostgresql non-standard-gid /var/lib/sepgsql sepgsql
> E: sepostgresql non-standard-dir-perm /var/lib/sepgsql 0700
> E: sepostgresql non-standard-uid /var/lib/sepgsql/data sepgsql
> E: sepostgresql non-standard-gid /var/lib/sepgsql/data sepgsql
> E: sepostgresql non-standard-dir-perm /var/lib/sepgsql/data 0700
> E: sepostgresql non-standard-uid /var/lib/sepgsql/backups sepgsql
> E: sepostgresql non-standard-gid /var/lib/sepgsql/backups sepgsql
> E: sepostgresql non-standard-dir-perm /var/lib/sepgsql/backups 0700

The rpmlint generate same errors, except for paths, for the postgresql-server 
package.
I don't think it's a problem.

> W: sepostgresql incoherent-version-in-changelog 8.2.4-0.407 8.2.4-
0.407.beta.fc8

> W: sepostgresql dangerous-command-in-%postun userdel
As I mentioned above, it's commented out.

> E: sepostgresql subsys-not-used /etc/rc.d/init.d/sepostgresql
/etc/rc.d/init.d/sepostgresql uses pg_ctl to start/stop SE-PostgreSQL server 
process, and it creates lock file under the /var/lib/sepgsql.
Should it be fixed?

(In reply to comment #8)
> (In reply to comment #6)
> > did you successfully build this in mock?  there seem to be missing 
> buildrequires
> > when i attempt it.
> 
> Your patch intended to add "Requires(pre): shadow-utils", but it is not 
> necessary because we can assume some fundamental packages are installed.
> See, http://fedoraproject.org/wiki/Packaging/FullExceptionList
> The shadow-utils is also contained the list.

My patch was not meant to address missing BuildRequires.  It was intended to
follow the recipe from the newly ratified users and groups guidelines.

As far as I can tell, the following BuildRequires are missing:
autoconf readline-devel zlib-devel bison

This is a problem because once #250494 is resolved, and the appropriate object
classes are in the selinux policy, sepostgresql will still not build in the
Fedora buildsystem.

> > E: sepostgresql subsys-not-used /etc/rc.d/init.d/sepostgresql
> /etc/rc.d/init.d/sepostgresql uses pg_ctl to start/stop SE-PostgreSQL server 
> process, and it creates lock file under the /var/lib/sepgsql.
> Should it be fixed?

Good question- I am not sure.

Well, I will read this thread later.

Umm.. actually I am not familiar with SELinux and I don't know
how to deal with SELinux policy, so I may be missing a lot, however:

* What is the difference between that you ask dwalsh
  to include your SELinux policy of PostgreSQL and that we don't
  modify the default policy of PostgreSQL and release the different
  variant of PostgreSQL as SE-PostgreSQL? (Again I may be missing
  a lot, so please explain....)

  Putting several variants of the same package is generally unwilling.
  For example, once bugs (especially security issues) against
  one variants are reported, we have to ping all variants and check
  if all variants fixed the issues.
  

(In reply to comment #11)

Tasaka-san, Thanks for your comments.

SELinux is a feature implemented in the kernel. It hooks any system call to 
make a decision based on the security policy, whether it should be allowed, or 
not. It enables to control accesses onto resources managed by the kernel, like 
file, socket and so on.
However, the kernel cannot identify user space objects like a window of X, or a 
table of database. It means bare-SELinux cannot control accesses onto user 
space objects.

* What is differences between PostgreSQL with SELinux and SE-PostgreSQL
Both PostgreSQL and SE-PostgreSQL execute system calls to operate something. 
SELinux hooks them as mentioned above. In addition, SE-PostgreSQL hooks any SQL 
query to make a decision based on the policy, whether it should be allowed, or 
not. It means that the security policy enables to control accesses onto 
userspace objects including tables, columns and so on. PostgreSQL does not have 
such a mechanism, so any operation are out of scope of the security policy.
It is the most fundamental difference.

* The reason why I submit a patch for selinux-policy
SELinux has a feature of loadable policy package. It enables to plug/unplug a 
set of security policy without modification of policy source. But some kinds of 
policy are exception.
The definitions of object classes and access vectors are one of exception. It 
defines permissions related to a specific object type.
No need to say, object classes related to database is a new concept for 
SELinux, so these permissions are currently undefined.
The patch I posted complement these lacking definitions.
If these definitions are integrated into the base security policy, we can pack 
the rest of policy within RPM of SE-PostgreSQL. Thus, it is necessary the patch 
to be merged.

Created attachment 160709 [details]
mock build log of 8.2.4-0.409.beta on rawhide i386

Just from packaging issue:

* Patch0
--------------------------------------------------
Patch0: sepostgresql-%{version}-%{release}.patch
--------------------------------------------------
  - Please don't write in this way. This surely
    fails on F-7 FC-6 (i.e. except for rawhide) because
    release is defined as:
--------------------------------------------------
Release: 0.409%{?sepgextension}%{?dist}
--------------------------------------------------

* BuildRequires
  - rebuild fails at least on rawhide i386. At least 
    autoconf is missing for BuildRequires.

    Even after adding autoconf to BuildRequiers, rebuild
    still fails (build log attached). 
    I cannot proceed review without making srpm rebuilt..

* AutoProv: no
  - Why is this needed?

* CFLAGS
--------------------------------------------------
CFLAGS=`echo $CFLAGS|xargs -n 1|grep -v ffast-math|xargs -n 100`
--------------------------------------------------
  - This is redundant because Fedora's CFLAGS does not contain
    -ffast-math

* Macros
  - Please use macros correctly.
    /var -> %_localstatedir
    /usr/sbin -> %_sbindir

* Install usage
  - Please make sure that "install" "cp" commands keep timestamp.
    i.e. Use "-p" option when using "install" or "cp".

* For group/user adding scripts:
  (from http://fedoraproject.org/wiki/PackagingDrafts/UsersAndGroups :
   this is ratified and now this is not a draft)

(In reply to comment #8)
> Your patch intended to add "Requires(pre): shadow-utils", but it is not 
> necessary because we can assume some fundamental packages are installed.
> See, http://fedoraproject.org/wiki/Packaging/FullExceptionList
> The shadow-utils is also contained the list.
  - FullExceptionList is for BuildRequires, not for Requires.
    So adding "Requires(pre): shadow-utils" is still needed

* Initscripts Conventions
  (check the section "Services"
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets )
  - Add some Requires(pre) or so on according to the description
    written on above.

* Directory ownership
  - Please make it sure that all the directories newly creted
    by installing this package are surely owned by this package.
    Currently the following directories are not owned.
-----------------------------------------------------------
%{_datadir}/sepgsql/
%{_libdir}/sepgsql/
------------------------------------------------------------

* From the brief check of sepostgresql.init:
  - Usually the commands which are not within normal users' paths
    must be specified with full path (otherwith this will cause
    problems when invoked with sudo rpm -Fvh , for example)

BTW now rawhide has selinux-policy-3.0.5-1.fc8

(In reply to comment #13)

> * Patch0
> --------------------------------------------------
> Patch0: sepostgresql-%{version}-%{release}.patch
> --------------------------------------------------
>   - Please don't write in this way. This surely
>     fails on F-7 FC-6 (i.e. except for rawhide) because
>     release is defined as:
> --------------------------------------------------
> Release: 0.409%{?sepgextension}%{?dist}
> --------------------------------------------------

Is it possible to use an immediate version identifier instead of "${version}-%
{release}", like "Patch0: sepostgresql-0.409.patch" ?
If not so, we cannot identify differenct patches.

> * AutoProv: no
>   - Why is this needed?

It got unnecessary in the current version.
I'll remove it.

> * CFLAGS
> --------------------------------------------------
> CFLAGS=`echo $CFLAGS|xargs -n 1|grep -v ffast-math|xargs -n 100`
> --------------------------------------------------
>   - This is redundant because Fedora's CFLAGS does not contain
>     -ffast-math

It's just a copy from postgresql.spec.
I'll remove it.

> * Macros
>   - Please use macros correctly.
>     /var -> %_localstatedir
>     /usr/sbin -> %_sbindir

The translation of "/usr/sbin -> %_sbindir" also targets /usr/sbin/semodule?
In my understanding, the location of external commands don't have any 
relationship with %_prefix of sepostgresql.

I don't have any specific note for the rest of your comments.
I'll fix them. Please wait for updating the specfile.

Thanks,

(In reply to comment #15)
> The translation of "/usr/sbin -> %_sbindir" also targets /usr/sbin/semodule?
  Yes.
> In my understanding, the location of external commands don't have any 
> relationship with %_prefix of sepostgresql.
 - Actually policycoreutils spec file uses %_sbindir . Using proper
   macros is necessary because we have to make it sure that spec files in
   Fedora follow FHS. So we expect that semodule is under %_sbindir.
   Let's say %_prefix changes, then if macros are used properly, all the
   affected commands will change the location properly. If you still
   use /usr/sbin, it surely fails.

Created attachment 160820 [details]
specfile diff 0.409 to 0.410

Please upload full spec/srpm so that we can check them
easily :)

Comment on attachment 160820 [details]
specfile diff 0.409 to 0.410

I updated sepostgresql.spec as the attachment.
* Macros are removed from Patch0: line.
* "AutoProv: no" is removed.
* Several needed packages are added into BuildRequires. These are imported from
the postgresql package's description.
* Several needed packages are added into Requires(xxx).
* add '-p' option into cp and install command.
* Redundant script to strip -ffast-math is removed.
* unnecessary '/' is removed.
* '/var' -> %{_localstatedir}
* '/usr/sbin' -> %{_sbindir}
* using /sbin/service instead of /etc/init.d/%{name}
* %dir %{_datadir}/sepgsql and %dir %{_libdir}/sepgsql are added, for owning
directories.

(In reply to comment #18)
> Please upload full spec/srpm so that we can check them
> easily :)

SpecUrl: http://sepgsql.googlecode.com/files/sepostgresql.spec
SrpmUrl: http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-
0.410.beta.fc8.src.rpm

(In reply to comment #14)
> BTW now rawhide has selinux-policy-3.0.5-1.fc8

I found a problem with the selinux-policy-devel-3.0.5-1.fc8 as I reported to 
fedora-selinux list as follows:
http://marc.info/?l=fedora-selinux-list&m=118645893208232&w=2

Thus, the base policy of SE-PostgreSQL is still selinux-policy-3.0.4-1.fc8 for 
the rawhide.

Created attachment 160828 [details]
mock build log of 410

This time many files are installed but not listed.

(In reply to comment #22)
> mock build log of 410
> This time many files are installed but not listed.

These unpacked files are not necessary for the sepostgresql package.

SE-PostgreSQL provides several features as a database server, like as 
postgresql-server package provides, so we don't need to pack all of files 
installed, such as frontend utilities, HTML documentation and header and 
library files.

In the native postgresql case, these are also unpacked in postgresql-server 
package. Rest of subpackages contain them.

Removing them makes harder to maintain the package, so I 
applied "_unpackaged_files_terminate_build" configuration.

Thanks,

Well, actually unless bug 250494 is resolved, it seems that
I cannot go further on this review request??

However for initscripts:
* lock file and pid file
  - Usually when daemon is running, there should be two files
    - /var/run/<daemon name>.pid - contains the info of the pid
                                   number
    - /var/lock/subsys/<daemon name>
    (Please refer to
     http://fedoraproject.org/wiki/FCNewInit/Initscripts 
     Although I am not sure to what degree we should follow
     these guidelines... and "Init Script Functions" seems to
     be supported only partially)

* status
  - and what does "service sepostgresql status" return?
    Usually the format is:
----------------------------------------------------
[root@localhost ~]# LANG=C service xfs status
xfs (pid 2343) is running...
----------------------------------------------------

Created attachment 161040 [details]
init script of sepostgresql-8.2.4-0.418.beta.fc8

Here is the new version of SE-PostgreSQL
SpecURL: http://sepgsql.googlecode.com/files/sepostgresql.spec
SrpmURL:
http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-0.418.beta.fc8.src.rpm

(In reply to comment #24)
> Well, actually unless bug 250494 is resolved, it seems that
> I cannot go further on this review request??

Yes, it depends on selinux-package with object classes definition for
SE-PostgreSQL, so it has to be resolved.
Fortunatelly, these new object classes are merged into the upstreamed reference
policy yesterday.
I think it will be integrated within the selinux-policy package soon.
  http://marc.info/?l=selinux&m=118666527208126&w=2

The new version of SE-PostgreSQL contains some modifications of the init
script.
> However for initscripts:
> * lock file and pid file
>   - Usually when daemon is running, there should be two files
>     - /var/run/<daemon name>.pid - contains the info of the pid
>				     number
>     - /var/lock/subsys/<daemon name>
I added a code to create lock file and pid file on startup, and to remove them
on shutdown. rpmlint got a silent.

I uses "/var/lock/subsys/${NAME}.lock" as a pathname of the lock file.
It is same manner with postgresql's one, but rpmlint made warnings.
Should it be replaced by an immediate value?

> * status
>   - and what does "service sepostgresql status" return?
>     Usually the format is:
> ----------------------------------------------------
> [root@localhost ~]# LANG=C service xfs status
> xfs (pid 2343) is running...
> ----------------------------------------------------

It displays the following message.
------------------------------------------
[root@masu ~]# service sepostgresql status
sepostgresql: server is running (PID: 11726)
[root@masu ~]# service sepostgresql stop
Stopping sepostgresql service:				   [  OK  ]
[root@masu ~]# service sepostgresql status
sepostgresql: no server running
[root@masu ~]# echo $?
3
[root@masu ~]#
------------------------------------------

In addition, I updated the path of the commands run by /sbin/runuser as
follows:
| cd ${SEPGSQL_BIN}
| /sbin/runuser sepgsql -- -c "./pg_ctl -D ${SEPGSQL_DATA} status"

When /sbin/runuser is run with CWD sepgsql cannot access, typically /root, a
noisy warnning message will be generated.
"cd ${SEPGSQL_BIN}" ensure that sepgsql can be placed on his accesable CWD.

SpecURL: http://sepgsql.googlecode.com/files/sepostgresql.spec
SrpmURL: http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-
0.427.beta.fc8.src.rpm

The SE-PostgreSQL package is updated.
It contains the following fixes.

1. A bug in the sepostgresql security policy module was fixed.
  An administration domain could execute a function with sepgsql_user_proc_t.
  The policy developer intend to deny it.

2. A script in the specfile was fixed.
  The name of the SELinux object classes are changed.
  We appanded "db_" prefix onto any object classed related to SE-PostgreSQL 
  while we have a discussion in the selinux-list.
  Therefore, "SECCLASS_DATABASE" is also renamed to "SECCLASS_DB_DATABSE",
  and a script to calculate its value in %build section is changed.

3. sepostgresql-pg_dump-renaming.patch is added
  pg_dumpall calls pg_dump internally, however, SE-PostgreSQL package installs
  an enhanced pg_dump(all) as sepg_dump(all) to avoid confliction with the
  native postgresql package.
  This small patch enables sepg_dumpall to use sepg_dump.

FYI:

dwalsh has a plan to merge the upstreamed reference policy into the selinux-
policy package on the next Monday.
It contains the required definitions of object classes/permission, so we will 
restart the reviewing process soon. :)

(In reply to comment #27)
> FYI:
> 
> dwalsh has a plan to merge the upstreamed reference policy into the selinux-
> policy package on the next Monday.

Good news!
Then I will recheck this package again.

(When new selinux-policy is published, please update your
 spec/srpm.
 And you are splitting writing srpm URL into two lines? Please
 write srpm URL in one line (even if it is long) so that we can
 download it easily)

SpecURL: http://sepgsql.googlecode.com/files/sepostgresql.spec
SrpmURL: http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-
0.428.beta.fc8.src.rpm

The SE-PostgreSQL package is updated.
It depends on selinux-policy-3.0.6 or latest, not a SE-PostgreSQL specific one.

(In reply to comment #29)
>  And you are splitting writing srpm URL into two lines?
Unfortunatelly, it is splitted into two lines implicitly. :(

[Spec URL]
http://sepgsql.googlecode.com/files/sepostgresql.spec
[SRPM URL]
http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-0.428.beta.fc8.src.rpm

# just a test to avoid splitting, please ignore.

Now selinux-policy 3.0.6-1.fc8 is rebuilt.
I am checking comment 31 now, however:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package may be accepted with another few (or some) 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 requests 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/PackageMaintainers/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 package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


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

Well, for 428:

= rebuild
  - rebuild seems okay on all archs (i386 x86_64 ppc ppc64)
    http://koji.fedoraproject.org/koji/taskinfo?taskID=123684

* Scriptlets requirement
  - As %post and %postun calls commands in policycoreutils,
    "Requires(post)" and "Requires(postun)" should also have
    policycoreutils.

* macros in %changelog
  - Please don't use macros in changelog. If you want to
    write .fc8 in %changelog, please write it explicitly
    (normally, we simply omit %dist part)
    https://www.redhat.com/archives/fedora-devel-list/2007-August/msg01580.html

Other things are okay. Also check my comment 32.

I updated the packeges as follows:
[Spec URL]
http://sepgsql.googlecode.com/files/sepostgresql.spec
[SRPM Url]
http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-0.429.beta.fc8.src.rpm

- "policycoreutils" is added into Requires(post) and Requires(postun)
- Any macro are simply removed from %changelog

(In reply to comment #32)
> Once you are sponsored, you have the right to review other 
> submitters' review requests and approve the packages formally. 

OK, I'll join the reviewing for several packages.

(In reply to comment #34)
> (In reply to comment #32)
> > Once you are sponsored, you have the right to review other 
> > submitters' review requests and approve the packages formally. 
> 
> OK, I'll join the reviewing for several packages.

Well, just to confirm that I want to see your pre-review or your another
review request _before_ I approve this package.

At first, I posted my reviews for two PostgreSQL related packages.

Bug #251805: postgresql-orafce - Implementation of some Oracle functions into 
PostgreSQL
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=251805

Bug #246782: postgresql-plr - Procedural language interface between PostgreSQL 
and R
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246782

(In reply to comment #35)
> Well, just to confirm that I want to see your pre-review or your another
> review request _before_ I approve this package.

Is it correct for your intention?

(In reply to comment #36)
> (In reply to comment #35)
> > Well, just to confirm that I want to see your pre-review or your another
> > review request _before_ I approve this package.
> 
> Is it correct for your intention?

Actually "before".

Okay.
* spec/srpm good
* rebuilds properly
  http://koji.fedoraproject.org/koji/taskinfo?taskID=125954
* can be installed properly, seems to work well
* from very quick check your pre-review seems good

-------------------------------------------------------------
   This package (sepostgresql) is APPROVED by me
-------------------------------------------------------------

Please follow the procedure according to:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor (at the stage, please also write on
this bug for confirmation that you requested for sponsorship)
Then I will sponsor you.

If you want to import this package into Fedora 7, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on Fedora rebuilding system).

If you have questions, please ask me.

Now I am sponsoring you.

(In reply to comment #37)
> Okay.
> * spec/srpm good
> * rebuilds properly
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=125954
> * can be installed properly, seems to work well
> * from very quick check your pre-review seems good
> -------------------------------------------------------------
>    This package (sepostgresql) is APPROVED by me
> -------------------------------------------------------------

Thanks,

> Please follow the procedure according to:
> http://fedoraproject.org/wiki/PackageMaintainers/Join
> from "Get a Fedora Account".
> At a point a mail should be sent to sponsor members which notifies
> that you need a sponsor (at the stage, please also write on
> this bug for confirmation that you requested for sponsorship)
> Then I will sponsor you.

I got a Fedora account as "kaigai", and made a request to belong to
"cvsextras" and "fedoradebugs" group.
(And it was approved during I'm writing this comment.)

I have to set up the Build-System Client Tools next, don't you?

> If you want to import this package into Fedora 7, you also have
> to look at
> http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
> (after once you rebuilt this package on Fedora rebuilding system).
> If you have questions, please ask me.

Unfortunatelly, it is not possible, because the selinux-policy package in 
Fedora 7 does not contains the definitions of object classes and permissions, 
as mentioned in Bug #250494 .

(In reply to comment #39)
> I got a Fedora account as "kaigai", and made a request to belong to
> "cvsextras" and "fedoradebugs" group.
> (And it was approved during I'm writing this comment.)
> 
> I have to set up the Build-System Client Tools next, don't you?
Yes, please go on.

> > If you want to import this package into Fedora 7, you also have
> > to look at
> > http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
> > (after once you rebuilt this package on Fedora rebuilding system).
> > If you have questions, please ask me.
> 
> Unfortunatelly, it is not possible, because the selinux-policy package in 
> Fedora 7 does not contains the definitions of object classes and permissions, 
> as mentioned in Bug #250494 .
Okay.

New Package CVS Request
=======================
Package Name: sepostgresql
Short Description: Security-Enhanced PostgreSQL
Owners: kaigai
Branches:
InitialCC:
Cvsextras Commits: no

New Package CVS Request
=======================
Package Name: sepostgresql
Short Description: Security-Enhanced PostgreSQL
Owners: kaigai
Branches:
InitialCC:
Cvsextras Commits: no

----
I'm wondering whether the New Package CVS Request is delivered correctly, 
because the Comment #41 has not appeared on the fedora-package-review list for　
half of a day.
(Is there something related to yesterday's maintenance of the Bugzilla?)

I see that this package "Conflicts: postgresql-server". Why? 
Can't you simply add a 'se' prefix on to conflicting filenames from that package? 
See: http://fedoraproject.org/wiki/Packaging/Conflicts

In particular: "There are many types of files which can conflict between
multiple packages. Fedora strongly discourages using Conflicts: to resolve these
cases."

It is not possible obviously.
The PostgreSQL implimentation which SE-PostgreSQL is based on uses its program 
name internally, so it is hard to prove the renaming gives us no degrading.

In addition, SE-PostgreSQL has to work for the database clients as if native 
PostgreSQL works on. Therefore, SE-PostgreSQL provides its service using the 
same network port (5432) to avoid client side modifications, but we cannot use 
both packages in same time.
It is a "implicit conflicts" with postgresql-server package.

Thanks,

So Kevin, what prevents this from getting imported?

Rereading the Conflicts Guidelines with your comments in mind I think I see a
point of confusion.  The "Implicit Conflicts" section highlights the fact that
implicit conflicts are never acceptable.  It says that all conflicts must be
explicit (marked with Conflicts: PKG).  If I understand, your reading of that
section is what leads you to mark the package Conflicts: postgresql-server.

However, that section, taken in context with the preceding paragraph is meant
only to convey that implicit conflicts are disallowed.  To use "Conflicts: PKG"
you still need to fit under one of the other categories (presently "Other
Functionality" or "Compat Packages") which this package does not.

This package comes closest to the "Binary Name Conflicts" section.  In that
section, two alternatives are given: Rename the files (in this case you would
also have to use a different port) or use alternatives to manage the dual install.

This package is a prime candidate for alternatives as it creates binaries that
are commandline compatible with postgresql-server and needs to use the same
port.  You'll need to talk to Tom Lane (tgl redhat.com; now CC'd), the
postgresql maintainer about how he feels about doing that with the main postgres
package.  He might also have a different idea on how to make the packages co-exist.

If you and tgl decide that Conflicts really is the best way to go, present your
reasoning to the Packaging Committee (fedora-packaging redhat.com) so they can
consider adding another case where "Conflicts: PKG" is allowed. 

As a side note: I think that using rm to clean up files that you don't want to
include in the package is much cleaner than using
  %define _unpackaged_files_terminate_build 0
Turning off the check is using a bigger stick than necessary.  Turning off the
check means that you won't be warned of cases where the build changes and starts
creating differently named binaries or new files that you actually want to
install.  Using rm will target specific files so it's more future proof.

(In reply to comment #46)
> Rereading the Conflicts Guidelines with your comments in mind I think I see a
> point of confusion.  The "Implicit Conflicts" section highlights the fact that
> implicit conflicts are never acceptable.  It says that all conflicts must be
> explicit (marked with Conflicts: PKG).
I might have a bit misunderstandings.

> This package comes closest to the "Binary Name Conflicts" section.  In that
> section, two alternatives are given: Rename the files (in this case you would
> also have to use a different port) or use alternatives to manage the dual 
install.

If possible, I want to avoid to apply the renaming approach, because it requires
us unnecessary differences from the native PostgreSQL, and the difference may
degrade the quality of package.

However, it seems to me that using alternatives is also overdoing approach,
because SE-PostgreSQL is a software to provide an advanced experimental feature
and has much less users than the native PostgreSQL.

> This package is a prime candidate for alternatives as it creates binaries that
> are commandline compatible with postgresql-server and needs to use the same
> port.  You'll need to talk to Tom Lane (tgl redhat.com; now CC'd), the
> postgresql maintainer about how he feels about doing that with the main 
postgres
> package.  He might also have a different idea on how to make the packages co-
exist.
> If you and tgl decide that Conflicts really is the best way to go, present 
your
> reasoning to the Packaging Committee (fedora-packaging redhat.com) so they can
> consider adding another case where "Conflicts: PKG" is allowed. 

I also want to hear his opinion.

In my opinion, we should add the third case to apply Conflicts: tag.
When the package provides an experimental feature based on existing package,
it can use Conflicts: tag to separate from the native package.

It may correspond with the filename conflicts cases, but applying alternatives
between the experimental package and the native package is like as comparison
between a giant and an ant. It's not a realistic way.

> As a side note: I think that using rm to clean up files that you don't want to
> include in the package is much cleaner than using
>   %define _unpackaged_files_terminate_build 0
> Turning off the check is using a bigger stick than necessary.  Turning off the
> check means that you won't be warned of cases where the build changes and 
starts
> creating differently named binaries or new files that you actually want to
> install.  Using rm will target specific files so it's more future proof.

At the first, I didn't use "_unpackaged_files_terminate_build", but I had to put
massive 'rm' commands.
I can remove it and remove unnecessary files, but I don't know whether it is
simple, or not.

Thanks,

Trying to conflict with just one subpackage of the postgresql group seems like a pretty bad idea.  For 
example, the PL and -test subpackages Require: postgresql-server, which would mean you'd have to 
duplicate those as se-specific versions.  (Which maybe you have to do anyway, for the PLs ... are they 
going to be binary-compatible?  What about postgresql-contrib?)

Using alternatives is a possibility, but I'm not real excited about it because of its invasive effects on the 
regular postgresql package.  I've also been reminded the hard way recently that RPM is not good about 
package upgrades that involve replacing files/directories with symlinks or vice versa; I'm afraid we'd hit 
one of those gotchas while trying to upgrade a postgresql installation into an alternatives-based setup.

I don't see the reason why you can't make it install parallel files with different names (sepostgres etc).
The argument that some error messages include the program name seems a bit silly, and it's been 
awhile since there were any hard dependencies on the executable name.

Rather than Conflicts: postgresql-server, I wonder whether you shouldn't be trying to Require: 
postgresql-server = %{version} so that you can share whichever files are in common, instead of 
shipping duplicates.

(In reply to comment #48)
> For example, the PL and -test subpackages Require: postgresql-server, which
> would mean you'd have to duplicate those as se-specific versions.

Indeed, you are correct. It's a demerit of using Conflict: tag.

> I don't see the reason why you can't make it install parallel files with
> different names (sepostgres etc).
> The argument that some error messages include the program name seems a bit
> silly, and it's been awhile since there were any hard dependencies on the
> executable name.

I don't say it is not possible, but I was anxious about this change may
invoke unexpected problems.

pg_ctl and initdb invoke the hardcoded "postgres", so we have to patch them
in the sepostgresql package at least.

Just I confirmed the /usr/bin/postgres source. Several codes seems invoke
the hardcoded "postgres", but most of them are enclosed by "#ifdef 
EXEC_BACKEND",
so it seems to me the binary name does not give us any significant affect.

OK, I try to rename the binary, and confirm its works.

> Rather than Conflicts: postgresql-server, I wonder whether you shouldn't be
> trying to Require: postgresql-server = %{version} so that you can share
> whichever files are in common, instead of shipping duplicates.

Maybe, we can omit any libraries (like utf8_and_euc_jp.so) and timezone data.

Thanks,

[SpecURL]
http://sepgsql.googlecode.com/files/sepostgresql.spec
[SrpmURL]
http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-0.432.beta.fc8.src.rpm

I updated the sepostgresql package as follows:
- several binary files are renamed
 * /usr/bin/initdb     -> /usr/bin/initdb.sepgsql
 * /usr/bin/pg_ctl     -> /usr/bin/sepg_ctl
 * /usr/bin/postgres   -> /usr/bin/sepostgres
 * /usr/bin/postmaster -> /usr/bin/postmaster
 * /usr/bin/pg_dump    -> /usr/bin/sepg_dump
 * /usr/bin/pg_dumpall -> /usr/bin/sepg_dumpall
   (pg_dump and pg_dumpall also have "se" prefix in older version)
- add Requires: postgresql-server, instead of Conflicts: postgresql-server
- several duplicate files with postgresql-server are removed,
  such as /usr/bin/ipcclean, /usr/lib/sepgsql/*.so and manpages in section 1.
- shared loadable modules are deployed on /usr/lib/pgsql to share them.
- "%define _unpackaged_files_terminate_build 0" is removed

In my simple test, it seems to work fine.

Could you review and check the specfile again?
I modified it a bit drastically.

Thanks,

(In reply to comment #50)
> - several binary files are renamed
>  * /usr/bin/postmaster -> /usr/bin/postmaster
                            ^^^^^^^^^^^^^^^^^^^
                            /usr/bin/sepostmaster
Sorry, it's a typo.

FWIW, you could probably drop sepostmaster altogether --- that symlink is only kept around for 
compatibilty with pre-8.2 startup scripts.  If you're going to teach a launch script to start sepostmaster, 
you might as well teach it to start sepostgres, and not bother with the extra symlink.

I'm going to unset the fedora-cvs flag on here. 
Once everything is hashed out, please reset it for the cvs request. 

Also I reset fedora review flag to question.

[SpecURL]
http://sepgsql.googlecode.com/files/sepostgresql.spec
[SrpmURL]
http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-0.433.beta.fc8.src.rpm

(In reply to comment #52)
> FWIW, you could probably drop sepostmaster altogether --- that symlink
> is only kept around for compatibilty with pre-8.2 startup scripts.

Your pointed out is fair enough.
I dropped "/usr/bin/sepostgresql" in the updated package.

(In reply to comment #55)
> I dropped "/usr/bin/sepostgresql" in the updated package.

Oops, it was "/usr/bin/sepostmaster", no need to say.

[SpecURL]
http://sepgsql.googlecode.com/files/sepostgresql.spec
[SrpmURL]
http://sepgsql.googlecode.com/files/sepostgresql-8.2.4-0.434.beta.fc8.src.rpm

(In reply to comment #48)
> For example, the PL and -test subpackages Require: postgresql-server,
> which would mean you'd have to duplicate those as se-specific versions.
> (Which maybe you have to do anyway, for the PLs ... are they going to
> be binary-compatible?  What about postgresql-contrib?)

The binary-compatibilities strongly depends on the internal implementation.
Because SE-PostgreSQL modifies several data structure like HeapTupleHeader
to store its security attribute, we cannot share *.so files in binary-
compatible.
Therefore, these are deployed on "/usr/lib/sepgsql" in the updated version,
as the previous one.
This manner follows "Library Name Conflicts" in the Conflicts Guidelines.

As you mentioned, when we use any other PL- or contrib modules, these also
have to be built for SE-PostgreSQL. However, these are not duplicate ones.

I want the latest one to be reviewed and be put on the next phase again.
Thanks,

Hmm ... if you changed HeapTupleHeader then it is pretty much completely unsafe to load *any* standard 
extension module into sepostgres; we have no way to tell which ones depend on that data structure.  But I 
think all the PLs do, and many contrib modules do, so none of those RPMs are compatible and you need to 
supply substitutes.

I wonder if it'd be feasible to hide the sepostgres add-on data into the t_hoff padding, the way OIDs have 
been handled the last few releases?  It's a bit of a long shot, but otherwise we're going to have a real hard 
time with binary dependencies.

(In reply to comment #58)
> I wonder if it'd be feasible to hide the sepostgres add-on data into
> the t_hoff padding, the way OIDs have been handled the last few releases?
> It's a bit of a long shot, but otherwise we're going to have a real hard 
> time with binary dependencies.

In SE-PostgreSQL, all tuples have a its security attribute without any 
exception,
and I made a decision it's not necessary this field with flexible length.
It is the reason why I didn't use t_hoff padding to store security attribtue.

Your suggestion seems to me fine enough. However, I hesitate to apply it onto
the 8.2.x based SE-PostgreSQL now, because this change affects so wide range.
I want this change to be queued for 8.3.x based SE-PostgreSQL.

In the current version, I believe that installing se- version module into
"/usr/lib/sepgsql" is the best way to enhance its functionality.

Thanks,

Personally, I'm against pushing this package to Fedora. SEPostgreSQL is not
tested widely yet -- and it is thought as a replacement of original PostgreSQL.
If people, who are already running their databases on PostgreSQL, wants to
switch to SEPostgreSQL and if it is broken somewhere, it will harm the good
reputation of PostgreSQL in terms of stability.

If someone calls for a vote, I strongly object to adding this package to Fedora
/ EPEL.

0.02 cents.

Regards, Devrim

(In reply to comment #60)
I can admit that any software contains bugs more or less.
However, I cannot understand your opinion.
In generally, if people who are already running their database on PostgreSQL,
switch to SE-PostgreSQL and if it is broken somewhere, they will switch back
it to the original PostgreSQL.
No need to say, it will be a reputation of SE-PostgreSQL, not original one.

In addition, getting merged into Fedora encourage widely test of SE-PostgreSQL.
I think it should be merged to resolve your concern also.

Hi,

(In reply to comment #61)
> (In reply to comment #60)
> In generally, if people who are already running their database on PostgreSQL,
> switch to SE-PostgreSQL and if it is broken somewhere, they will switch back
> it to the original PostgreSQL.

Ouch! So you don't care about the downtime? :(

> In addition, getting merged into Fedora encourage widely test of 
> SE-PostgreSQL. I think it should be merged to resolve your concern also.

Fedora is *not* for testing packages, IMHO. 

Regards, Devrim

(In reply to comment #60)
> Personally, I'm against pushing this package to Fedora. SEPostgreSQL is not
> tested widely yet -- and it is thought as a replacement of original PostgreSQL.

i disagree that SE-PostgreSQL is thought of as a replacement of PostgreSQL. 
furthermore, that concern could be addressed with adding sufficient warning to
SE-PostgreSQL's %description.

> If people, who are already running their databases on PostgreSQL, wants to
> switch to SEPostgreSQL and if it is broken somewhere, it will harm the good
> reputation of PostgreSQL in terms of stability.

i think the reasonable conclusion is that SE-PostgreSQL is broken somewhere and
to switch back to PostgreSQL.

Fedora is on the leading edge of SELinux development, and thus is a perfect
place to test this extension of PostgreSQL.

(In reply to comment #63)
Rob, thanks for your comment.
I also think your opinions are correct.

(In reply to comment #63)
I agree with rob.

People will think postgreSQL and SE-PostgreSQL is different.
People who want postgreSQL usally installs usual postgreSQL.
People who install SE-PostgreSQL will know SE-PostgreSQL have different feature.

>furthermore, that concern could be addressed with adding sufficient warning to
>SE-PostgreSQL's %description.
I agree this too, to avoid misunderstanding.

Hi,

(In reply to comment #63)
> i disagree that SE-PostgreSQL is thought of as a replacement of PostgreSQL. 
> furthermore, that concern could be addressed with adding sufficient warning to
> SE-PostgreSQL's %description.

How many people read the %description parts, guys?

> i think the reasonable conclusion is that SE-PostgreSQL is broken somewhere 
> and to switch back to PostgreSQL.

As I wrote before, this means a downtime , and usually There Is No Time For
Downtime.

> Fedora is on the leading edge of SELinux development, and thus is a perfect
> place to test this extension of PostgreSQL.

Fedora is not the place to test a replacement for PostgreSQL. 

We are talking about a database server guys, not a browser, or text editor, or such.

Regards, Devrim

(In reply to comment #66)
It is not a generic situation to replace a significant working system
by an unknown package, without enough evaluation and verification.
Especially, if it is required to minimize its downtime.

In addition, we are understanding that Fedora project positively fetch
many advanced and experimental features. These experimences are leveraged
in Red Hat Enterprise Linux and feedbacked to the later versions of Fedora.

No production server should be using rawhide. Whoever uses rawhide knows that
there is an experimental part over there. Therefore I think that rawhide is a
very food test place for SE-PostgreSQL. But the package should not be pushed to
stable without performing extensive testing first.

I like to return the topic about package reviewing.

At the point of Comment #57, I added Requires: tag with postgresql-server
instead of Conflicts: tag, and several binaries are renamed with "se" prefix
or ".sepgsql" postfix.
"%define _unpackaged_files_terminate_build 0" is removed, and all unnecessary
files became to be removed during %install section.

These fixes are reflecting to the above comments.

Thanks,

Looks pretty good.  What about the port?  Is that still the same as postgresql
server?

(In reply to comment #70)
> Looks pretty good.  What about the port?
> Is that still the same as postgresql server?

Yes. We have to stop postgresql server before running sepostgresql,
and vice versa.

I think it should be same in default, because we have to update widespread
database applications to connect sepostgresql if it use an alternative port.
In addition, it can be overwritten with "/etc/sysconfig/sepostgresql", if 
necessary.

Thanks,

Well, there's precedent for having two packages with the same port in things
like httpd/lighttpd packages I suppose.  All of the issues I raised have been
addressed.  Thanks!  I'm not running rawhide so I don't have a machine to test
that the packages still function after these changes.

mtasaka, would you like to make sure the packages still run and do the honors of
finishing off the review?

(In reply to comment #72)
> Well, there's precedent for having two packages with the same port in things
> like httpd/lighttpd packages I suppose.  All of the issues I raised have been
> addressed.  Thanks!  

Okay, thank you, Kuratomi-san.
I will then recheck the recent srpm, mainly for packaging issue (if
any).

Okay for me.

Again I APPROVE this package.

Thanks for your reviewing again.

New Package CVS Request
=======================
Package Name: sepostgresql
Short Description: Security-Enhanced PostgreSQL
Owners: kaigai
Branches:
InitialCC:
Cvsextras Commits: no

cvs done. Thank you for addressing the Conflicts. I think this is a much nicer
package now. :) 

Please close this bug as NEXTRELEASE when rebuild is done.