GSConnect / gnome-shell-extension-gsconnect

KDE Connect implementation for GNOME
GNU General Public License v2.0
3.1k stars 253 forks source link

Use GioUnix for GNOME 46 #1782

Closed ferdnyc closed 2 months ago

ferdnyc commented 3 months ago

Seems Gio has been fragmented, its platform-specific classes are now part of a separate GioUnix library.

Fixes #1781

ferdnyc commented 3 months ago

I suspect our CI image needs to be updated to include the GioUnix library.

Test log file gsconnect_plugins_testSftpPlugin.test.txt:

(gjs:406): Gjs-CRITICAL **: 11:36:48.325: JS ERROR: 
Error: Requiring GioUnix, version none: 
Typelib file for namespace 'GioUnix' (any version) not found
@/usr/libexec/installed-tests/gsconnect/fixtures/backend.js:8:17
@/usr/libexec/installed-tests/gsconnect/fixtures/utils.js:33:26
@/usr/libexec/installed-tests/gsconnect/plugins/testSftpPlugin.js:7:15
initTests@/usr/libexec/installed-tests/gsconnect/minijasmine:128:5
@/usr/libexec/installed-tests/gsconnect/minijasmine:162:1

(gjs:406): Gjs-CRITICAL **: 11:36:48.325: 
Script /usr/libexec/installed-tests/gsconnect/minijasmine 
threw an exception
ferdnyc commented 3 months ago

Hmmm. Unfortunately, even in Fedora 39, GioUnix isn't available.

(/usr/lib64/libgio-2.0.so.0 lives in the glib2 package, /usr/lib64/girepository-1.0/Gio-2.0.typelib lives in gobject-introspection, and /usr/share/gir-1.0/Gio-2.0.gir lives in gobject-introspection-devel, but none of those include GioUnix in Fedora 39. Presumably Fedora 40 will have it available, but that's still in beta so I don't think there are Docker images yet. This may have to wait until release. ...Which is soon, so NBD.)

ferdnyc commented 3 months ago

Presumably Fedora 40 will have it available, but that's still in beta so I don't think there are Docker images yet.

Hm! Shows what I know, the fedora/fedora repo on quay.io even has fedora:41 images! (That's the current rawhide bleeding-edge branch that will become Fedora 41 come November.)

So in theory we could switch to using a Fedora 40 beta image (which includes GNOME 46) for the CI.

andyholmes commented 2 months ago

Ah right, I forgot about this. It's purely a namespace change in terms of API, so if necessary I think polyfill/alias can be used.

https://gitlab.gnome.org/GNOME/gjs/blob/83003b6c4781735818f195da7c2cb61059bcd4c1/installed-tests/js/testGDBus.js#L7-L15

// Adapter for compatibility with pre-GLib-2.80
let GioUnix;
if (imports.gi.versions.GioUnix === '2.0') {
    GioUnix = imports.gi.GioUnix;
} else {
    GioUnix = {
        InputStream: Gio.UnixInputStream,
    };
}
ferdnyc commented 2 months ago

@andyholmes That should work, plus OutputStream for nativeMessagingHost.js.

It's weird, though, that running under the new CI image didn't fix this — I even see in the Meson logs that it's GLib 2.80, so that should have GioUnix available. I even checked on koji earlier that its .gir and .typelib files are included in the F40 gobject-introspection[-devel] RPMs.

Maybe I'll pull the new CI image locally and take a look.

andyholmes commented 2 months ago

Yeah, my local fedora-toolbox:40 definitely has it, and I double-checked the container hash with the GSConnect/gsconnect-ci:main hash. :thinking:

ferdnyc commented 2 months ago

Un-marking this as WIP because it could be merged now, though for the record I am planning on updating the GioUnix uses to take a "polyfill" approach to loading.

But if anyone's getting impatient and doesn't feel like waiting for that change, this is now mergeable as-is, since the CI image is updated to GNOME 46 and builds are once again in the green.

andyholmes commented 2 months ago

Either way is fine, it's not critical either way. Merge when ready :)

ferdnyc commented 2 months ago

@andyholmes

Ooh, actually, I don't see how this would work:

// Adapter for compatibility with pre-GLib-2.80
let GioUnix;
if (imports.gi.versions.GioUnix === '2.0') {
    GioUnix = imports.gi.GioUnix;
} else {
    GioUnix = {
        InputStream: Gio.UnixInputStream,
    };
}

My code is setting imports.gi.versions.GioUnix = '2.0'; early on in daemon.js, since that's the (weird) method gobject-introspection uses to do version selection. So the condition's always going to be true.

Would a try/catch be a safer approach? Testing in the gjs console, it seems to work:

let GioUnix;
try {
    GioUnix = imports.gi.GioUnix;
} catch (e) {
    GioUnix = {
        InputStream: Gio.UnixInputStream,
        OutputStream: Gio.UnixOutputStream,
    };
}
andyholmes commented 2 months ago

Actually, it might be good to have @retrixe weigh in here, since a polyfill/alias might need another approach for ESModules.

andyholmes commented 2 months ago

Ooh, actually, I don't see how this would work:

Hmm, actually for old-school imports I bet this would work:

let GioUnix = null;
try {
    GioUnix = imports.gi.GioUnix;
} catch {
    GioUnix = imports.gi.Gio;
}

The versioning probably isn't necessary; I think it's questionable whether there will ever be a another major revision of the GLib/GObject/Gio and there certainly isn't now.

ferdnyc commented 2 months ago

Hmm, actually for old-school imports I bet this would work:

let GioUnix = null;
try {
    GioUnix = imports.gi.GioUnix;
} catch {
    GioUnix = imports.gi.Gio;
}

Alas, nope. The member names are UnixInputStream and UnixOutputStream in Gio but InputStream and OutputStream in GioUnix.

andyholmes commented 2 months ago

Ah, disappointing :(

retrixe commented 2 months ago

For ESM, we would need to use dynamic imports with top level await in a try/catch

For specifying the version, we could either specify the version every time we import it, or use dynamic import in try/catch where we currently set the version

ferdnyc commented 2 months ago

Don't ask me how I copy-pasted a block of code to three files, yet managed to leave out a space in only one of them.

ferdnyc commented 2 months ago

@andyholmes

The versioning probably isn't necessary; I think it's questionable whether there will ever be a another major revision of the GLib/GObject/Gio and there certainly isn't now.

That part I certainly agree with. (Doubly so for GJS, where versions are forced to 2.0 for things like GLib and Gio "since we depend on them internally".)

I'd been given the impression that setting explicit versions kept the import code from doing lots of extra work to figure out the available versions and picking which one to load. But now that I'm reading the code, I'm questioning that. Seems like it does lots of work regardless, calling enumerate_versions() and scanning the repository even if a version was explicitly selected.

andyholmes commented 2 months ago

This looks good to merge from my side, whenever you're ready.

I'd suggest we merge this before #1683, so there's a commit to branch/release off of if necessary.

ferdnyc commented 2 months ago

Sorry, was asleep at the wheel. Trigger pulled.