Bug 189662
Summary: | Review Request: transconnect -- A function imposter to allow transparent connection over HTTPS proxies | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Enrico Scholz <rh-bugzilla> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | opensource |
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: | 2009-03-10 17:55:28 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: | 201449 |
Description
Enrico Scholz
2006-04-22 11:01:57 UTC
You should follow the naming guidelines, even if there is never a l.3, there is no reason to use 1.3. Also I think you should drop the %release_func stuff and do things more simply. * Mon Jul 24 2006 Enrico Scholz <enrico.scholz.de> - 1:1.3-0.1.Beta - follow Fedora naming guidelines strictly; increased epoch http://ensc.de/fedora/transconnect/ ==== > Also I think you should drop the %release_func stuff and do things > more simply. no; I like it this way * Mon Jul 24 2006 Enrico Scholz <enrico.scholz.de> - 1:1.3-0.2.Beta - enhanced -fixup patch to put a optimization barrier after cleaning password data http://ensc.de/fedora/transconnect/ (In reply to comment #2) > - follow Fedora naming guidelines strictly; increased epoch This package have never been released, you don't need to increase the epoch? > ==== > no; I like it this way Indeed, but if I'm not wrong %{!?release_func:%global release_func() %1%{?dist}} Release: %release_func 0.2.Beta leads to the much simpler Release: 0.2.Beta%{?dist} so why not use the simpler way? Of course you could have set release_func() somewhere, but this flexibility is not usefull in fedora extras, yet it adds some complexity to the spec file. Otherwise, I have some other comments: * I think it would be nice if you dropped a line somewhere (in README.fedora?) explaining that tconn-localres.so corresponds with the make localres case described in the INSTALL file, or something along those lines. * I also think that the file tconn.cat could be modified such that export LD_PRELOAD=$HOME/.tconn/tconn.so is replaced by export LD_PRELOAD=%{_libdir}/tconn.so * I know that some reviewer disagree on having modules to be dlopened directly in %_libdir and insist on having dlopened modules in subdirs of %_libdir. I tend to agree with that when the modules are 'internal modules' or backends to be used by an application or a library. The transconnect case is less clear in my opinion, however I still think that it would be cleaner if the *.so where in a %_libdir subdir, say %{_libdir}/transconnect this would imply some adjustments in the tconn script, and in tconn.cat there should then be export LD_PRELOAD=%{_libdir}/transconnect/tconn.so What do you think about that? * Tue Jul 25 2006 Enrico Scholz <enrico.scholz.de> - 1:1.3-0.3.Beta - removed the %(id -u) from the buildroot; it adds unneeded clutter, is not required and you gain nothing with it - fixed paths in 'tconn.cat' - enhanced 'README.fedora' http://ensc.de/fedora/transconnect/ ====== > > - follow Fedora naming guidelines strictly; increased epoch > > This package have never been released, you don't need to increase > the epoch? Needed as upgrade path on my systems; you want to follow the guidelines strictly and these guidelines do not forbid epoch... > > no; I like it this way > > Indeed, but if I'm not wrong > %{!?release_func:%global release_func() %1%{?dist}} > Release: %release_func 0.2.Beta > > leads to the much simpler > Release: 0.2.Beta%{?dist} > > so why not use the simpler way? * I like it * it is used in all my other packages * it does not violate the guidelines > * I think it would be nice if you dropped a line somewhere (in > README.fedora?) explaining that tconn-localres.so corresponds > with the make localres case described in the INSTALL file, or > something along those lines. ok; added some lines > * I also think that the file tconn.cat could be modified such that > export LD_PRELOAD=$HOME/.tconn/tconn.so > is replaced by > export LD_PRELOAD=%{_libdir}/tconn.so ok; is now 'LD_PRELOAD=tconn.so' > * I know that some reviewer disagree on having modules to be > dlopened directly in %_libdir and insist on having dlopened > modules in subdirs of %_libdir. 'tconn.so' will not be dlopen()'ed but LD_PRELOAD'ed. Placing into the searchpath allows | LD_PRELOAD=tconn.so without specifying the full path. Therefore, I will keep it in %_libdir. (In reply to comment #5) > * Tue Jul 25 2006 Enrico Scholz <enrico.scholz.de> - 1:1.3-0.3.Beta > - removed the %(id -u) from the buildroot; it adds unneeded clutter, > is not required and you gain nothing with it This issue should really be clarified. In the guidelines it is said "The preferred value for the BuildRoot tag is" so this looks like an almost must to me. I'll ask on the fedora-extras-list > - fixed paths in 'tconn.cat' > - enhanced 'README.fedora' Ok. > > This package have never been released, you don't need to increase > > the epoch? > > Needed as upgrade path on my systems; you want to follow the guidelines > strictly and these guidelines do not forbid epoch... I think it is better to follow the guidelines, because this is a case where they make sense, since there may be a 1.3 and there is no reason to make an exception for this package anyway. Adding an epoch is also bad. I also understand your local issue, but I think we should avoid as far as possible that local issue interfer with fedora extras packaging. Isn't there another solution for your upgrade path, like setting a Provide temporarily in your local repo? Otherwise I guess this issue should be submitted to the fedora-extras-list, since it is likely to be a situation that will happen again in the future. > * I like it > * it is used in all my other packages > * it does not violate the guidelines If you insist. But I think that it really complicates things needlessly. > 'tconn.so' will not be dlopen()'ed but LD_PRELOAD'ed. Placing into the > searchpath allows > > | LD_PRELOAD=tconn.so > > without specifying the full path. Therefore, I will keep it in %_libdir. Ok, makes sense. (In reply to comment #6) > (In reply to comment #5) > This issue should really be clarified. In the guidelines it is said > "The preferred value for the BuildRoot tag is" > so this looks like an almost must to me. I'll ask on the fedora-extras-list It has been clarified, so it is right as is. The only issue remaining is the release. arglll... I forgot this review completely :( > The only issue remaining is the release. mmh... you said >> * I like it >> * it is used in all my other packages >> * it does not violate the guidelines > If you insist. But I think that it really complicates things needlessly. which sounds like "I do not like it but go ahead when you really want" for me... And I really want... (In reply to comment #8) > which sounds like "I do not like it but go ahead when you really want" for me... That's the right interpretation ;-) However for the release this is currently "I do not like it and for me it is a blocker". But I haven't assigned the bug to me such that somebody else can accept what you propose... Anybody working on this? Is this included in Fedora? Is alternative of this software with similar functionality included in Fedora? I have rebuild the SRPM from the links above on Fedora8 without any issues. Will test how it works. The problem is that no reviewier will approve this package as-is because of weirdness in the specfile. It's unfortunate that a recalcitrant packager can hold up the process like this; if someone else wanted to submit a package without such weirdness then I would be happy to review it. It may also happen that some reviewer accepts the release and macros stuff even though it is weird. Apart from that the package is right if I remember well. Issues at first sight: - GPL is not a valid License shortname anymore - SourceX files are not prefixed with %{name}: README.fedora, tconn - The comment before the Version-tag does not apply anymore - The patch should be commented with the explanation, that upstream is dead - The correct SF.net Source0 URL is: http://downloads.sourceforge.net/%{name}/%{name}-%{version}-Beta.tar.gz - It looks to me, that it contains libraries in %libdir, therefore these scriptlets need to be run: %post -p /sbin/ldconfig %postun -p /sbin/ldconfig If this is not needed, please explain why. This has been needinfo since last year; closing. |