Bug 41790 - Make file transfer support optional
Summary: Make file transfer support optional
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/an...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-10-14 07:08 UTC by Andre Moreira Magalhaes
Modified: 2011-12-22 06:07 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Andre Moreira Magalhaes 2011-10-14 07:08:52 UTC
Gabble now depends on libnice just for GTalk compatible FT support. It would be good to make the libnice dependency optional.
Comment 1 Andre Moreira Magalhaes 2011-10-14 07:09:26 UTC
Branch at URL attempts to fix.
Comment 2 Will Thompson 2011-10-31 09:23:51 UTC
+    AC_SUBST(NICE_CFLAGS)
+    AC_SUBST(NICE_LIBS)
+else
+    NICE_CFLAGS=
+    NICE_LIBS=
+fi

If you're not AC_SUBSTing the empty variables, can't you just leave the else clause out entirely?

I don't really like how this patch litters quite so many ifdefs around, even down to changing the number of arguments to gabble_file_transfer_channel_new()…
Comment 3 Andre Moreira Magalhaes 2011-10-31 09:51:05 UTC
(In reply to comment #2)
> +    AC_SUBST(NICE_CFLAGS)
> +    AC_SUBST(NICE_LIBS)
> +else
> +    NICE_CFLAGS=
> +    NICE_LIBS=
> +fi
> 
> If you're not AC_SUBSTing the empty variables, can't you just leave the else
> clause out entirely?
Actually I should AC_SUBST them every time, will fix this

> I don't really like how this patch litters quite so many ifdefs around, even
> down to changing the number of arguments to gabble_file_transfer_channel_new()…
Tbh, I don't like it either, but I don't see how it could be any better, suggestions?
Comment 4 Will Thompson 2011-11-10 07:35:54 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > +    AC_SUBST(NICE_CFLAGS)
> > +    AC_SUBST(NICE_LIBS)
> > +else
> > +    NICE_CFLAGS=
> > +    NICE_LIBS=
> > +fi
> > 
> > If you're not AC_SUBSTing the empty variables, can't you just leave the else
> > clause out entirely?
> Actually I should AC_SUBST them every time, will fix this
> 
> > I don't really like how this patch litters quite so many ifdefs around, even
> > down to changing the number of arguments to gabble_file_transfer_channel_new()…
> Tbh, I don't like it either, but I don't see how it could be any better,
> suggestions?

I guess it would be clearer if there were something like a subclass of GabbleFTChannel for GTalk (well, maybe GabbleFTChannel would be an abstract base class and there would be two subclasses, one for GTalk and one for SI transfers), maybe?
Comment 5 Marco Barisione 2011-11-11 01:56:29 UTC
How about just being able to disable FT completely?
Comment 6 Andre Moreira Magalhaes 2011-12-20 16:04:52 UTC
(In reply to comment #5)
> How about just being able to disable FT completely?

Ok, as agreed, we decided to go this route, so hijacking this bug.

URL updated to point to a new branch "ft-optional" that adds an option to disable ft completely.

I tried to use as few ifdefs as possible.
Comment 7 Jonny Lamb 2011-12-21 07:01:29 UTC
Seems reasonable enough.

How come you didn't just conditional out all the file transfer tests in tests/twisted/Makefile.am instead of skipping each one if FT is not enabled?
Comment 8 Andre Moreira Magalhaes 2011-12-22 05:30:43 UTC
(In reply to comment #7)
> Seems reasonable enough.
> 
> How come you didn't just conditional out all the file transfer tests in
> tests/twisted/Makefile.am instead of skipping each one if FT is not enabled?

At first, I made the tests conditional in tests/twisted/Makefile.am, but then I saw other tests that were being skipped due to some disabled stuff, so I did the same as I guess this was intended. See twisted/sidecars.py, jingle/google-relay.py, jingle/call-*, etc.
Comment 9 Jonny Lamb 2011-12-22 05:39:42 UTC
(In reply to comment #8)
> At first, I made the tests conditional in tests/twisted/Makefile.am, but then I
> saw other tests that were being skipped due to some disabled stuff, so I did
> the same as I guess this was intended. See twisted/sidecars.py,
> jingle/google-relay.py, jingle/call-*, etc.

OK. Merge away.
Comment 10 Andre Moreira Magalhaes 2011-12-22 06:07:28 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > At first, I made the tests conditional in tests/twisted/Makefile.am, but then I
> > saw other tests that were being skipped due to some disabled stuff, so I did
> > the same as I guess this was intended. See twisted/sidecars.py,
> > jingle/google-relay.py, jingle/call-*, etc.
> 
> OK. Merge away.

Thanks, merged. It will be in gabble 0.15.4.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.