Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 168210
Summary: | Review Request: phpldapadmin - Web-based tool for managing LDAP servers | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Dmitry Butskoy <dmitry> |
Component: | Package Review | Assignee: | Aurelien Bompard <gauret> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-extras-list |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://phpldapadmin.sourceforge.net | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2005-09-27 12:04:24 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 |
Description
Dmitry Butskoy
2005-09-13 15:27:31 UTC
Upgrade to 0.9.7-rc2 New SRPM: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin-0.9.7-rc2.src.rpm New SPEC: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin.spec Typo in url... New SRPM: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin-0.9.7-0.2.rc2.src.rpm New SPEC: http://dmitry.butskoy.name/phpldapadmin/phpldapadmin.spec 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 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. 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. - 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? 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. > 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. 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. > 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. > 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. > 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? :)
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. 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. 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)? 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. 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 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. (for comment #18) I agree. All issues are in active discussion now in upstream devel list... Package Change Request ====================== Package Name: phpldapadmin New Branches: EL-4 EL-5 cvs done. |