Bug 168210 - Review Request: phpldapadmin - Web-based tool for managing LDAP servers
Review Request: phpldapadmin - Web-based tool for managing LDAP servers
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Aurelien Bompard
David Lawrence
http://phpldapadmin.sourceforge.net
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-09-13 11:27 EDT by Dmitry Butskoy
Modified: 2007-12-10 11:39 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-09-27 08:04:24 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dmitry Butskoy 2005-09-13 11:27:31 EDT
Spec Url: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin.spec
SRPM Url: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin-0.9.7-0.1.20050912cvs.src.rpm

Description: 

PhpLDAPadmin is a web-based LDAP client.
It provides easy, anywhere-accessible, multi-language administration
for your LDAP server. Its hierarchical tree-viewer and advanced search
functionality make it intuitive to browse and administer your LDAP directory.

Since it is a web application, this LDAP browser works on many platforms,
making your LDAP server easily manageable from any location.

PhpLDAPadmin is the perfect LDAP browser for the LDAP professional
and novice alike. Its user base consists mostly of LDAP administration
professionals.


Additional info:

Package layout is "derived" from squirrelmail

CVS snapshot (20050912) is one week later after 0.9.7-rc1 is released. 0.9.7 is an upcoming release, it is enough reasons to use it. Snapshot is needed due to some little typos in rc1 .
While this package is reviewed, the rc2 will be released, etc... :-)

Config file (patch by me) is oriented for LDAP server at the same host where http server is run. In this case there is nothing to manually configure.
Comment 3 Dmitry Butskoy 2005-09-20 13:08:11 EDT
Upgrade to 0.9.7-rc3, and add some postinstall script.

New SRPM: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin-0.9.7-0.3.rc3.src.rpm
New SPEC: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin.spec




Comment 4 Aurelien Bompard 2005-09-24 11:33:33 EDT
Review for release 0.3.rc3:
* RPM name is OK
* Source phpldapadmin-0.9.7-rc3.tar.gz is the same as upstream
* Patches look OK
* Builds fine in mock
* rpmlint of phpldapadmin looks OK
* File list of phpldapadmin looks OK
* Works fine

Please update to rc4 before or after importing, and you're good to go.
Comment 5 Aurelien Bompard 2005-09-24 11:39:38 EDT
Just one more thing. Please add this to your apache conf file:
<Directory /usr/share/phpldapadmin>
  Order Deny,Allow
  Deny from all
  Allow from 127.0.0.1
</Directory>

We don't want phpldapadmin to be available to anybody by default.
Comment 6 Dmitry Butskoy 2005-09-26 10:41:34 EDT
- Upgrade to final 0.9.7
- Add patches which are not yet committed upstream.
- Generally remove all the debug stuff (it gives essential speedup of work).
- Initially, allow connect from localhost only.


  Aurelien,

  Look at it just in case once again:
New SRPM: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin-0.9.7-1.src.rpm
New SPEC: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin.spec

  BTW, we install apache conf file (under conf.d/) -- how to tell Apache that it
has appeared? 
Comment 7 Aurelien Bompard 2005-09-26 11:56:38 EDT
Yes, big update indeed.

> - Add patches which are not yet committed upstream.
> - Generally remove all the debug stuff (it gives essential speedup of work).

In Fedora, we try to be as close to upstream as possible, and thus, to include
the minimum of patches. Please follow this policy. It looks like patches 3 and 4
are not essential, what are they for ?
Also, please drop the debug_log pruning, it can be useful.

> how to tell Apache that it has appeared?

Apache will see it when it is reloaded, but we usually don't do that in %post or
%postun. We let the admin do it when he wants.
Comment 8 Dmitry Butskoy 2005-09-26 12:44:10 EDT
> In Fedora, we try to be as close to upstream as possible, and thus, to include
> the minimum of patches. Please follow this policy. It looks like patches 3 and 4
> are not essential, what are they for ?
Both of them are pending in upstream. IMHO, upstream maintainers just had not
time to engage them, hurrying up to release the final version. At least, patch 3
has been discussed and approved (probably for the next release).

Patch3 allows to disable blowfish encryption of php session variables. (However
it is not a default in the rpm). Such disabling increases reactivity of PLA a
little. It is not allowed for 'cookie' authentication stuff, but allowed for
'session'. It is a behaviour of previous phpldapadmin versions (<= 0.9.5)

Patch4 completes "show_clear_password" feature. In the current upstream, when it
is enabled, some things are still shown by asterisks that is needless.

I provide these patches since PLA-0.9.3 . Both of them do not affect default
behaviour. I would prefer to include it...


> Also, please drop the debug_log pruning, it can be useful.

Sadly, but current debug_log() application leads to essential delay of work (up
to 3-4 times on some operations). This issue is posted upstream too.

IMO, the problem is inspired by a lot of
"debug_log(sprintf(a_lot_of_arguments......));" constructions, where "sprintf()"
is caused irrespective of the debug level.


  Aurelien,

  If you insist, I'm ready to refise patch 3 and 4 (until upstream commits
them). But i'm confident that we shouls leave debug stripping.

  As I yet don't think the final upstream version (0.9.7) is stable enough, I
would prefer to take this package in the devel branch only. Usually, soon after
the stable a first upstream update is appeared (aka 0.9.7a etc. :-)). At this
moment, I hope, I'll finish my discussions with upstream. Besides somebody will
already take advantage of phpldapadmin as a rpm package. It will be a good
moment to add PLA into other branches (FC3, FC4) too.




Comment 9 Aurelien Bompard 2005-09-26 13:10:52 EDT
I'd prefer not to include those patches in the rpm, and just wait until they are
included upstream. That's Fedora's policy in general (especially true when
applied to the kernel). I understand that those are your patches, and submitting
them upstream was the right move. Let's just wait for them to be official, we're
bleeding-edge enough.

The problem with the debug_log function pruning is that I don't want to see this
kind of scenario in upstream's bug tracker :
"Please send your logs. Ah, you're using Fedora ? Well, CLOSED WONTFIX".
If the issue is posted upstream too, couldn't we wait for it to be resolved
there ? It would probably be done in a cleaner way (eg: put the sprintf lower on
the stack, and keep the debug)
For regular packages, we have the -debuginfo rpm if we need debug. Here, the
debug is gone for good.

IMHO, patches should almost only be used for Fedora-specific customizations.

Concerning the devel branch, that's not a problem, just request branches when
you judge it stable enough.
Comment 10 Dmitry Butskoy 2005-09-26 15:52:35 EDT
> I'd prefer not to include those patches in the rpm, and just wait until they are
> included upstream. 
OK.
But it is quite possible, that these patches will be included in a cvs branch
focused on some following version (0.9.8), which is necessary to wait about one
year... :( Terms of patch reviewing, as well as terms of new version releasing
are unpredictable enough.

> The problem with the debug_log function
debug_log() stuff in the present kind has appeared in the current version only.
There was no any debug stuff in phpldapadmin <= 0.9.5, therefore I've paid
attention to essential delay of work of the new version (0.9.7) in comparison
with old (0.9.5). Application of debug_log() looks as an internal tool for
upstream developers only, whom have forgotten to erase it before release :) 

PLA has good bug report mechanism. Immediately in the error report's screen the
user is invited to fill in a form, which can be sent upstream. It is a preferred
way of reporting bugs for years.

> Here, the debug is gone for good.
Yes, due to too big cost of such a debug.


  Aurelien,

  Whether it is a good idea:
- to include nevertheless phpldapadmin package in the kind suggested by me into
devel branch only;
- and then report about it upstream?
  It will create some precedent and will push upstream developers. Besides they
can participate in testing their soft, which have been made out (IMHO, for the
first time!) in the form of a RPM package. Anyway, they will see problems
arising at attempts to package their soft.
Comment 11 Aurelien Bompard 2005-09-26 16:07:54 EDT
> Terms of patch reviewing, as well as terms of new version releasing
> are unpredictable enough

In any case, it is not ours to decide when these patches should be made public,
it's upstream's decision.

> debug_log() stuff in the present kind has appeared in the current version only

Alright, you've convinced me. But please, please, don't do it with this
incomprehensible awk script. Even perl would be clearer. As written in
PackageReviewGuidelines, and with all due respect, Fedora is no place for
entries into http://www.ioccc.org

Under these terms, you'll have my APPROVE vote for inclusion in CVS. Feel free
to request branches whenever you see fit. Just submit an updated srpm here so
that I can take a final look, please.
Comment 12 Dmitry Butskoy 2005-09-26 16:42:47 EDT
> But please, please, don't do it with this
> incomprehensible awk script. Even perl would be clearer.
As for you awk, so for me perl is incomprehensible :-) 
Unfortunately I'm not a perl programmer. It can seem strange, but awk is enough
for me (at least for a while). Besides that, as I remember, awk is POSIX'ed and
considered some basic UNIX tool.

I hope debug_log() issue will be resolved somehow in the near future, and the
script will gone for good.
May I leave it for a while? :)


Comment 13 Aurelien Bompard 2005-09-26 17:21:00 EDT
I'm not sure I would have preferred perl anyway (I'm a python guy)...
OK then. I've run the script, it looks like it does what it's supposed to do.
Go ahead and import.
Comment 14 Aurelien Bompard 2005-09-27 05:40:16 EDT
Maybe I wasn't clear enough: I think patches 3 and 4 should NOT be included, for
policy reasons. Looking at the cvs import log, it seems that you've imported
them. Please remove them, or at least remove them from the spec file.
Comment 15 Dmitry Butskoy 2005-09-27 07:46:05 EDT
Sorry for misunderstanding...

I just comment out these patches in the %prep section. Or delete it completely?

One more problem: I've already built this package. Better rebuild it (without
patches)?
Comment 16 Aurelien Bompard 2005-09-27 08:04:24 EDT
Please comment out the patches in %prep and the corresponding "PatchX:" tag, and
remove them from CVS (it's no use if they're not applied and will be included
upstream).
Then, bump the release and request a rebuild.
Comment 17 Dmitry Butskoy 2005-09-27 08:36:53 EDT
Done.

If these patches will be applied upstream, I shall backport them (if new
upstream version will be delayed).

Aurelien,
Thanks for careful review!
Comment 18 Michael Schwendt 2005-10-03 07:23:39 EDT
Comment 9 is a good point.  While practically we are permitted to modify the
source code and the program, we ought to be really careful about not annoying
upstream developers with changes which differ from their official release. What
the packager might consider useful from the perspective of a software user,
might not be the developers' point of view. At least ask them for permission for
applying the patches in Fedora Extras, and include their response and the
rationale for the patches as %doc.
Comment 19 Dmitry Butskoy 2005-10-03 07:41:42 EDT
(for comment #18)
I agree.

All issues are in active discussion now in upstream devel list...

Comment 20 Dmitry Butskoy 2007-12-10 07:47:21 EST
Package Change Request
======================
Package Name: phpldapadmin
New Branches: EL-4 EL-5
Comment 21 Kevin Fenzi 2007-12-10 11:39:37 EST
cvs done.

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