Created attachment 102924 [details] [review]
patch to rhythmbox for libsoup 2.4

Ok, after some trouble compiling RB with glib 2.15.3, I managed to have it up and running with the patch, and it seems to work just fine. 

*** Bug 513263 has been marked as a duplicate of this bug. ***

can this please be reviewed and committed within this month?

This can't be committed as is, we need to keep compatibility with libsoup 2.2 as well (with #ifdef)

okay. just keep in mind that GNOME 2.22 has been entirely ported to libsoup 2.4, so distros may want to get all the other apps, like rhythmbox, ported also.

(In reply to comment #6)
> okay. just keep in mind that GNOME 2.22 has been entirely ported to libsoup
> 2.4, so distros may want to get all the other apps, like rhythmbox, ported
> also.

FWIW, Fedora 9 is going to have a libsoup22 compat package (and it looks like gentoo is doing the same thing too: bug 512810). I've been working on porting various non-GNOME-release packages that use libsoup to libsoup 2.4, but there are still probably others that aren't in Fedora that no one is going to end up writing patches for until post-GNOME-2.20.

(In reply to comment #7)
> (In reply to comment #6)
> > okay. just keep in mind that GNOME 2.22 has been entirely ported to libsoup
> > 2.4, so distros may want to get all the other apps, like rhythmbox, ported
> > also.
> 
> FWIW, Fedora 9 is going to have a libsoup22 compat package <snip>

But Rhythmbox in F9 won't be using it, we're using the patch from this bug.

I'm working on reviewing this and adding the #ifdef mess required to support libsoup 2.2 as well.

Just to make sure these were errors in the patch, rather than bugs/missing API in libsoup:

+		g_signal_connect (auth_domain, "authenticate",
+				  G_CALLBACK (soup_auth_callback), share);
+		soup_server_add_auth_domain (share->priv->server, auth_domain);
+		g_object_unref (auth_domain);

That signal doesn't exist (soup_auth_domain_basic_set_auth_callback does, though), and the server doesn't ref the auth domain when it's added, so unreffing it here causes weird crashes later.

Created attachment 104312 [details] [review]
updated patch, reinstates libsoup 2.2 support

I've tested this lightly with both 2.2 and 2.4 and things seem to be OK.

(In reply to comment #9)
> Just to make sure these were errors in the patch, rather than bugs/missing API
> in libsoup:
> 
> +               g_signal_connect (auth_domain, "authenticate",
> +                                 G_CALLBACK (soup_auth_callback), share);
> +               soup_server_add_auth_domain (share->priv->server, auth_domain);
> +               g_object_unref (auth_domain);
> 
> That signal doesn't exist (soup_auth_domain_basic_set_auth_callback does,
> though)

Er, right, sorry, that API change got made after I'd started working on the patches, and then since the code still compiled fine without warnings I didn't notice it needed to be fixed.

> and the server doesn't ref the auth domain when it's added, so
> unreffing it here causes weird crashes later.

Doh. That's a bug; the server should be reffing the domain. I'm going to fix that for 2.3.2, which means your new patch will leak the auth domain, but that's a small leak, and you can fix it later by upping the libsoup dependency in a future release.

OK, thanks.  I'll probably commit this patch in the next couple of days if no other issues come up.

The build fails with libsoup 2.3.0, and the pkg-config file seems to be called just "libsoup.pc" in the current SVN.

(In reply to comment #13)
> The build fails with libsoup 2.3.0

With missing soup-enum-types.h? Apparently the dependencies aren't listed
right in libsoup/Makefile.am, but it *usually* builds them in the right order.
If you "make -C libsoup soup-enum-types.h" by hand the rest should build. I'll
have it fixed in svn soon.

> and the pkg-config file seems to be called
> just "libsoup.pc" in the current SVN.

the install rule renames it when it installs it

(In reply to comment #14)
> (In reply to comment #13)
> > The build fails with libsoup 2.3.0
> 
> With missing soup-enum-types.h? Apparently the dependencies aren't listed
> right in libsoup/Makefile.am, but it *usually* builds them in the right order.
> If you "make -C libsoup soup-enum-types.h" by hand the rest should build. I'll
> have it fixed in svn soon.

No, rhythmbox fails to build against libsoup 2.3.0. You built my packages, I don't see why I should build it myself ;)

From http://koji.fedoraproject.org/koji/getfile?taskID=395918&name=build.log:
rb-audioscrobbler.c: In function 'rb_audioscrobbler_parse_response':
rb-audioscrobbler.c:706: error: 'SoupMessage' has no member named 'response'
rb-audioscrobbler.c: In function 'rb_audioscrobbler_do_handshake':
rb-audioscrobbler.c:914: warning: passing argument 4 of 'rb_audioscrobbler_perform' from incompatible pointer type
rb-audioscrobbler.c: In function 'rb_audioscrobbler_submit_queue':
rb-audioscrobbler.c:1067: warning: passing argument 4 of 'rb_audioscrobbler_perform' from incompatible pointer type

That's with the patch from comment 10.

> > and the pkg-config file seems to be called
> > just "libsoup.pc" in the current SVN.
> 
> the install rule renames it when it installs it

That's quite ugly.

(In reply to comment #15)
> From http://koji.fedoraproject.org/koji/getfile?taskID=395918&name=build.log:
> rb-audioscrobbler.c: In function 'rb_audioscrobbler_parse_response':
> rb-audioscrobbler.c:706: error: 'SoupMessage' has no member named 'response'

That's in the #else branch of an #if defined(HAVE_LIBSOUP_2_4) block, so it shouldn't be trying to compile that.. maybe I screwed up the configure.ac changes?  Is there a way to get at the config.log or config.h files from that build?

(In reply to comment #16)
> (In reply to comment #15)
> > From http://koji.fedoraproject.org/koji/getfile?taskID=395918&name=build.log:
> > rb-audioscrobbler.c: In function 'rb_audioscrobbler_parse_response':
> > rb-audioscrobbler.c:706: error: 'SoupMessage' has no member named 'response'
> 
> That's in the #else branch of an #if defined(HAVE_LIBSOUP_2_4) block, so it
> shouldn't be trying to compile that.. maybe I screwed up the configure.ac
> changes?  Is there a way to get at the config.log or config.h files from that
> build?

Never mind, I was missing a call to autoheader.

Created attachment 104484 [details] [review]
rb-more-soup-debug.patch

An additional patch to show which version of libsoup will be used.

The patch doesn't quite build with libsoup 2.4. There are two calls to
soup_message_add_header() in plugins/audioscrobbler/ that need to be changed
to soup_message_headers_append().

Also, a few other notes on the patch (none of these are critical):

in rb-audioscrobbler.c:
  +#if defined(HAVE_LIBSOUP_2_4)
  +           audioscrobbler->priv->status_msg = g_strdup (msg->reason_phrase);
  +#else
              audioscrobbler->priv->status_msg = g_strdup (soup_status_get_phrase (msg->status_code));
  +#endif

That shouldn't be #ifdeffed. Use the "msg->reason_phrase" version for both 2.2 and 2.4. (The original code in rhythmbox was basically wrong; msg->reason_phrase already has the text of the error message *as it was returned from the server*, so you should use that, rather than regenerating the standard error message for that status code, which may be less specific in some cases.)

in rb-lastfm-source.c and rb-daap-connection.c
  +#if defined(HAVE_LIBSOUP_2_2)
 	soup_message_set_http_version (msg, SOUP_HTTP_1_1);
  +#endif

This is also not 2.2/2.4 specific; HTTP/1.1 is defined to be the default in both libsoup 2.2 and 2.4, so this call is just a no-op, which is why I removed it my patch.

in rb-daap-connection.c:
  +/* g_base64_encode was introduced in glib 2.12 */
  +#if (GLIB_MINOR_VERSION > 11)
  +#define HAVE_G_BASE64_ENCODE
  +#endif

You don't really need a separate define for that, since if you're using libsoup 2.2, you can just use soup_base64_encode, and if you're using libsoup 2.4, you must have at least glib 2.15.0.

in rb-daap-share.c, message_set_from_rb_daap_structure()
  +#if defined(HAVE_LIBSOUP_2_4)
  +       soup_message_set_response (message, "application/x-dmap-tagged", SOUP_MEMORY_TAKE, resp, length);
  +#else
          message->response.owner = SOUP_BUFFER_SYSTEM_OWNED;
          message->response.length = length;
          message->response.body = resp;
 
  +       soup_server_message_set_encoding (SOUP_SERVER_MESSAGE (message), SOUP_TRANSFER_CONTENT_LENGTH);
  +#endif

The soup_message_set_response version of the code will work for 2.2 as well (though you'll still need the soup_server_message_set_encoding call in 2.2).

There's similar code in send_mapped_file() where you can use soup_message_set_response() for both versions as well, though you'll need an additional #define in rb-soup-compat.h mapping SOUP_MEMORY_TEMPORARY to SOUP_BUFFER_USER_OWNED.

In rb-daap-share.c, databases_cb:

  +#if defined(HAVE_LIBSOUP_2_2)
          path = soup_uri_to_string (soup_message_get_uri (message), TRUE);
  +#endif
  ...
  +#if defined(HAVE_LIBSOUP_2_4)
  +       } else if (g_ascii_strcasecmp ("/1/items", rest_of_path) == 0) {
  +#else
          } else if (g_ascii_strncasecmp ("/1/items?", rest_of_path, 9) == 0) {
  +#endif
  ...

You could simplify this by doing "path = soup_message_get_uri(message)->path" rather than "path = soup_uri_to_string(...)". That way it wouldn't have the query tacked on to the end, and so you could use the same comparisons as in the libsoup 2.4 codepath. (Although this would require changing how the "rest_of_path" stuff works, so maybe it would end up making the diff bigger in the end...)

(You might also be able to simplify code by having server_cb parse the uri->query into a hashtable the way libsoup 2.4 does, so that you'd only need a single codepath for the query-handling code in all of the other server callbacks.)

The attached patches fail with --as-needed

Thanks for your comments, Dan.

I don't really want to rework the libsoup 2.2 code much, as I'm hoping we can drop it fairly soon.

Created attachment 104523 [details] [review]
updated patch

FYI, patch against 0.11.4 at:
http://cvs.fedoraproject.org/viewcvs/rpms/rhythmbox/devel/soup24.patch

Bastien:

This patch also fails to build when using the --as-needed linker option. Check the order of libraries and object files:

i686-pld-linux-gcc -O2 -fno-strict-aliasing -fwrapv -march=i686 -mtune=pentium4 -gdwarf-2 -g2 -Wl,--as-needed -o .libs/rhythmbox main.o -pthread -pthread -pthread -pthread -Wl,--export-dynamic  ./.libs/librbshell.a ../corelib/.libs/librhythmbox-core.so -L/usr/lib /usr/lib/libsexy.so ../sources/.libs/libsourcesimpl.a ../podcast/.libs/librbpodcast.a ../plugins/.libs/librbplugins.a /usr/lib/libsoup-2.4.so /usr/lib/libgio-2.0.so /usr/lib/libgnutls.so /usr/lib/libtasn1.so /usr/lib/libgcrypt.so /usr/lib/libgpg-error.so /usr/lib/libnotify.so ../bindings/python/.libs/rb.a -lpython2.5 /usr/lib/libffi.so /usr/lib/libgnome-media-profiles.so ../backends/.libs/librbbackends.a /usr/lib/libgstpbutils-0.10.so -lc -lgcc_s /usr/lib/libtotem-plparser.so /usr/lib/libcamel-1.2.so -L/usr/lib/lib -lkrb5 -lk5crypto -lcom_err -lgssapi_krb5 -lssl3 -lsmime3 -lnss3 -lsoftokn3 /usr/lib/libedataserver-1.2.so -lplc4 -lplds4 -lnspr4 /usr/lib/libdb-4.6.so /usr/lib/libsasl2.so -lcrypt /usr/lib/libnautilus-burn.so /usr/lib/libhal.so /usr/lib/libgnomeui-2.so /usr/lib/libgnome-keyring.so /usr/lib/libjpeg.so /usr/lib/libbonoboui-2.so -lcap /usr/lib/libSM.so /usr/lib/libICE.so /usr/lib/libgnomecanvas-2.so /usr/lib/libgailutil.so /usr/lib/libgnome-2.so /usr/lib/libesd.so /usr/lib/libasound.so /usr/lib/libaudiofile.so /usr/lib/libpopt.so /usr/lib/libbonobo-2.so /usr/lib/libbonobo-activation.so /usr/lib/libORBitCosNaming-2.so /usr/lib/libart_lgpl_2.so /usr/lib/libglade-2.0.so /usr/lib/libgtk-x11-2.0.so /usr/lib/libgdk-x11-2.0.so /usr/lib/libatk-1.0.so /usr/lib/libgdk_pixbuf-2.0.so /usr/lib/libpangocairo-1.0.so /usr/lib/libXinerama.so /usr/lib/libXi.so /usr/lib/libXrandr.so /usr/lib/libXcursor.so /usr/lib/libXcomposite.so /usr/lib/libXext.so /usr/lib/libXdamage.so /usr/lib/libcairo.so -lpng12 /usr/lib/libxcb-render-util.so /usr/lib/libxcb-render.so /usr/lib/libXrender.so /usr/lib/libpangoft2-1.0.so /usr/lib/libfontconfig.so /usr/lib/libfreetype.so /usr/lib/libpango-1.0.so /usr/lib/libxcb-xlib.so /usr/lib/libxcb.so /usr/lib/libXfixes.so /usr/lib/libX11.so /usr/lib/libXau.so /usr/lib/libXdmcp.so /usr/lib/libgnomevfs-2.so /usr/lib/libdbus-glib-1.so -lssl -lcrypto /usr/lib/libavahi-glib.so /usr/lib/libavahi-client.so /usr/lib/libdbus-1.so /usr/lib/libavahi-common.so /usr/lib/libssp.so -lresolv -lutil /usr/lib/libgconf-2.so /usr/lib/libORBit-2.so /usr/lib/libgstbase-0.10.so /usr/lib/libmusicbrainz.so /usr/lib/libexpat.so /usr/lib/libstdc++.so /usr/lib/libgstcontroller-0.10.so /usr/lib/libgstreamer-0.10.so /usr/lib/libxml2.so /usr/lib/libgobject-2.0.so /usr/lib/libgthread-2.0.so -lpthread /usr/lib/libgmodule-2.0.so -ldl /usr/lib/libglib-2.0.so /usr/lib/libpcre.so -lselinux -lm -lrt -lz 
../corelib/.libs/librhythmbox-core.so: undefined reference to `soup_uri_new'
../corelib/.libs/librhythmbox-core.so: undefined reference to `soup_uri_set_password'
../corelib/.libs/librhythmbox-core.so: undefined reference to `soup_uri_set_scheme'
../corelib/.libs/librhythmbox-core.so: undefined reference to `soup_uri_set_host'
../corelib/.libs/librhythmbox-core.so: undefined reference to `soup_uri_set_user'
../corelib/.libs/librhythmbox-core.so: undefined reference to `soup_uri_set_port'
collect2: ld returned 1 exit status

Created attachment 104589 [details] [review]
might fix --as-needed?

(not that I've tested it now or will in the future)

Confirmed, fixes --as-needed

Committed to svn.  Thanks again, Dan.

-> RESOLVED FIXED ?

er, yeah.

diff --git a/lib/rb-proxy-config.h b/lib/rb-proxy-config.h
index 5e95819..60eb04e 100644
--- a/lib/rb-proxy-config.h
+++ b/lib/rb-proxy-config.h
@@ -65,9 +65,7 @@ GType		rb_proxy_config_get_type (void);
 
 RBProxyConfig *	rb_proxy_config_new (void);
 
-#if defined(HAVE_LIBSOUP)
-SoupUri *	rb_proxy_config_get_libsoup_uri (RBProxyConfig *config);
-#endif
+SoupURI *	rb_proxy_config_get_libsoup_uri (RBProxyConfig *config);
 
 #endif	/* RB_PROXY_CONFIG_H */
 

At least that hunk seems to be missing from svn

The problem was that I stupidly removed the '#if defined(HAVE_LIBSOUP)', so it didn't build if you don't have libsoup.  I've fixed it up now.

*** Bug 520680 has been marked as a duplicate of this bug. ***

Created attachment 107592 [details] [review]
soup_encode_uri changes

The committed patch doesn't contain the necessary changes to account for the soup_uri_decode api change, in 2.2 it was 
void soup_uri_decode (char *);
and in 2.4 it is
char *soup_uri_decode (const char *);
ie it used to modify its argument but it no longer does.
I think this is the reason why some of my submissions to last.fm started to show some %20 instead of spaces. The attached patch should fix that, but I only compile-tested it with libsoup 2.4. I'm about to test how submission works with it, compilation tests with libsoup 2.2 would be appreciated

oops, yeah that looks right to me.

(now might also be a good time to bump the libsoup-2.4 requirement up to >= 2.4.0 and fix the leak mentioned in comment #11 if you haven't yet.)

Tested with libsoup 2.2 and 2.4, works fine.

Committed