Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updatechecker: Port to libsoup3 and small other fixes #1336

Merged
merged 4 commits into from
Jul 14, 2024

Conversation

b4n
Copy link
Member

@b4n b4n commented Apr 24, 2024

Port to libsoup3 (see #1295 (comment) @jbicha) plus a couple other fixes I just couldn't leave there after seeing them 🙂

⚠️ DISCLAIMER: I don't know neither libsoup2.4 nor libsoup3. This is a uneducated guess by looking at the scarce documentation and the API reference. And it seems to work with limited testing (I tested manually only, for the normal case, the 404 case and the incorrect domain case -- all of which got the expected result).

@b4n b4n added this to the 2.1.0 milestone Apr 24, 2024
@b4n b4n requested review from elextr and frlan April 24, 2024 11:00
@elextr
Copy link
Member

elextr commented Apr 26, 2024

I don't know enough about libsoup to sensibly review (and don't have the time to learn), but since Geany uses GIO why not g_file_new_for_uri() and g_file_read() (or g_file_read_async()/g_file_read_finish() to not block the UI) instead of using libsoup directly?

@xiota
Copy link
Contributor

xiota commented Apr 26, 2024

g_file_new_for_uri() and g_file_read() (or g_file_read_async()/g_file_read_finish()

It works...

#include <gio/gio.h>
#include <stdio.h>

int main() {
  GFile *file = g_file_new_for_uri("https://geany.org/service/version/");

  GError *error = NULL;
  GFileInputStream *stream = g_file_read(file, NULL, &error);

  if (!error) {
    printf("No error.\n");

    guint8 *buffer = NULL;
    gsize size = 0;

    buffer = (guint8 *)g_malloc(32);

    gboolean success = g_input_stream_read_all((GInputStream *)stream, buffer,
                                               31, &size, NULL, &error);

    if (success) {
      buffer[size] = '\0';
      printf("Data: %s\n", buffer);
    } else {
      printf("Error reading stream.\n");
    }

  } else {
    printf("Error reading uri.\n");
  }

  return 0;
}
$ gcc test.c -o test $(pkgconf --libs gio-2.0) $(pkgconf --cflags gio-2.0)

$ ./test
No error.
Data: 2.0.0

@elextr
Copy link
Member

elextr commented Apr 26, 2024

@xiota neat, and simple, although it has one theoretical bug (though unlikely in practice) [hint: what if 32 bytes were returned].

Just for general information, Geany used (a long time ago) to avoid GIO, which is probably why the plugin used libsoup directly. But now GIO is required by Geany so its available to the plugin.

@xiota
Copy link
Contributor

xiota commented Apr 26, 2024

Edited code in previous comment to request 31 bytes from g_input_stream_read_all().

It was written to check whether https:// is supported because I saw only file:// mentioned in the docs.

@elextr
Copy link
Member

elextr commented Apr 26, 2024

Unfortunately the GIO docs don't say when g_file_new_from_URI() and the associated stream was supported (that I could see) but it works on the machine I'm on ATM which is pretty olde.

PS I expect not everyone has your alias pkgconf == pkg-config 😉

@xiota
Copy link
Contributor

xiota commented Apr 26, 2024

Unfortunately the GIO docs don't say when g_file_new_from_URI() and the associated stream was supported (that I could see) but it works on the machine I'm on ATM which is pretty olde.

Looks like it depends on gvfs... After uninstalling...

$ ./test
Error reading uri.

Maybe better to depend on gvfs than libsoup-3.0 because gvfs has a longer support history (works on LTS distros) than libsoup 3.

PS I expect not everyone has your alias pkgconf == pkg-config 😉

On my computer, pkg-config is the symlink. Looks like pkgconf and pkg-config are different projects.

@elextr
Copy link
Member

elextr commented Apr 26, 2024

Yeah, pkg-config is the Freedesktop original, pkgconf would appear to be a re-implementation. Your distro installs pkgconf and links the original command to it so existing scripts will work, if they happen to conform to what pkgconf considers "correct".

If an existing script fails because it depends on some edge case in pkg-config tough. And very aggressive about it they are, "if someone insists on fixing such a non-bug, this constitutes a violation of our Code of Conduct, which may be addressed by banning from this repository".

Edit: by "script" I really mean the .pc files that various apps install

@xiota
Copy link
Contributor

xiota commented Apr 26, 2024

I use whatever is provided by the package manager. Even on Debian, pkg-config is a dummy package that redirects to pkgconf. Fedora also does not appear to provide the original pkg-config.

Their stance doesn't seem particularly "aggressive" in context because they call out "passive-aggressive people who try to argue with us". Seems they probably had some people who opened numerous issues reporting closely related problems that they were told multiple times are not bugs. It would be like... what if someone opened multiple issues for Geany complaining that it doesn't support various Qt6 features despite being told multiple times that they aren't bugs? Eventually, those complaints could be considered intentionally disruptive, and people who "insist" on continuing need to be cut off.

@elextr
Copy link
Member

elextr commented Apr 26, 2024

Looks like it depends on gvfs... After uninstalling...

Well, IIUC GVFS is part of GIO (not to be confused with GnomeVFS), so if its uninstalled its not surprising parts of GIO don't work.

But that probably means reading URLs won't work on Windows, dunno about Macos, so back to the soup I guess 😜

@xiota
Copy link
Contributor

xiota commented Apr 26, 2024

It is separate from GIO. This is the project page: https://gitlab.gnome.org/GNOME/gvfs

Debian packages, appear to be gvfs and gvfs-backends.

Looks like msys2 and brew do not have gvfs.

@elextr
Copy link
Member

elextr commented Apr 26, 2024

Well, I interpret the very first words in the readme "GVfs is a userspace virtual filesystem implementation for GIO" to mean its part of GIO. IIUC it provides the implementations for the abstract file/URL/dbus stream operations in GIO and thats why you get an error if its not installed and you try to read anything other than a local file.

Never mind, its was worth a try, and thanks for writing that test program.

@xiota
Copy link
Contributor

xiota commented Apr 26, 2024

Someone could run the test program on windows or mac to find out. I no longer have (easy) access to either.

@b4n
Copy link
Member Author

b4n commented Apr 26, 2024

Yeah I guess using GIO directly for this kind of super simple GET would make sense Indeed. I also think that gvfs will indeed be part of all reasonable GIO installations, but I could be wrong.

@b4n
Copy link
Member Author

b4n commented Apr 26, 2024

hum… @elextr @xiota so what's your conclusion? Depend on libsoup3 as this PR because it's HTTP, or depend on a GVFS backend for HTTP?

@b4n
Copy link
Member Author

b4n commented Apr 26, 2024

Note that writing an async version of the feature, while reasonably easy, is a little bit more work.

@b4n
Copy link
Member Author

b4n commented Apr 26, 2024

See #1340 if wanted.

@elextr
Copy link
Member

elextr commented Apr 27, 2024

hum… @elextr @xiota so what's your conclusion? Depend on libsoup3 as this PR because it's HTTP, or depend on a GVFS backend for HTTP?

Waiting for #1340 (comment)

If it works ? #1340 : #1336

@b4n
Copy link
Member Author

b4n commented May 1, 2024

This PR won (IMO), so it's the one to review @frlan

b4n added 4 commits June 16, 2024 11:42
libsoup2 is phasing out, and using it in a plugin causes conflicts with
other plugins using a newer version, including ones using a current
webkit2gtk.
Also, properly abort pending requests when unloading the plugin, in the
unlikely case they didn't all finish by that time.
Not only should this be in the German translation if need be, but I'd
also just not use quotes after a colon.
@b4n b4n force-pushed the updatechecker-libsoup3 branch from 15f0182 to e1c9ac3 Compare June 16, 2024 09:42
@b4n
Copy link
Member Author

b4n commented Jun 16, 2024

I rebased this on master now #1342 is merged, as it has CI infrastructure changes for building with libsoup3. Apart from that it's unchanged from the previous version.

@b4n
Copy link
Member Author

b4n commented Jun 16, 2024

@frlan I think this should really land (or be rejected if need be) before 2.1 so we have libsoup3 support for every relevant plugin.

@eht16
Copy link
Member

eht16 commented Jun 16, 2024

I'll test the CI installers in the next days when time permits.

@eht16
Copy link
Member

eht16 commented Jun 16, 2024

Tested on Windows and works.

@b4n
Copy link
Member Author

b4n commented Jul 6, 2024

@frlan IMO we need this for 2.1, see #1336 (comment). I'll merge this in a few days unless somebody complains.

@frlan frlan merged commit c53695d into geany:master Jul 14, 2024
2 checks passed
@b4n b4n deleted the updatechecker-libsoup3 branch July 24, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants