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 ReviewAssignee: 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: rawhideCC: 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
Spec URL: http://ensc.de/fedora/transconnect/transconnect.spec
SRPM URL: http://ensc.de/fedora/transconnect/
Description: 

Transconnect is an implementation to allow network connections over a
HTTP proxy. This should work under almost all linux distributions
using glibc, and all proxies allowing https CONNECT (eg squid).

Comment 1 Patrice Dumas 2006-07-23 21:47:31 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.

Comment 2 Enrico Scholz 2006-07-23 23:52:52 UTC
* 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

Comment 3 Enrico Scholz 2006-07-24 00:08:33 UTC
* 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/


Comment 4 Patrice Dumas 2006-07-24 08:22:03 UTC
(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?


Comment 5 Enrico Scholz 2006-07-25 13:13:28 UTC
* 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.

Comment 6 Patrice Dumas 2006-07-25 14:37:31 UTC
(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.

Comment 7 Patrice Dumas 2006-07-25 21:43:22 UTC
(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.



Comment 8 Enrico Scholz 2006-09-10 18:33:35 UTC
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...

Comment 9 Patrice Dumas 2006-09-10 19:54:43 UTC
(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...

Comment 11 Georgi Hristov 2008-01-21 16:47:53 UTC
Anybody working on this? Is this included in Fedora? Is alternative of this
software with similar functionality included in Fedora?

Comment 12 Georgi Hristov 2008-01-21 17:34:51 UTC
I have rebuild the SRPM from the links above on Fedora8 without any issues. Will
test how it works.

Comment 13 Jason Tibbitts 2008-01-21 18:01:58 UTC
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.

Comment 14 Patrice Dumas 2008-01-21 19:33:49 UTC
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.

Comment 15 Till Maas 2008-12-10 12:32:59 UTC
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.

Comment 16 Jason Tibbitts 2009-03-10 17:55:28 UTC
This has been needinfo since last year; closing.