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

Use GioUnix for GNOME 46 #1782

Merged
merged 4 commits into from
Apr 8, 2024
Merged

Use GioUnix for GNOME 46 #1782

merged 4 commits into from
Apr 8, 2024

Conversation

ferdnyc
Copy link
Member

@ferdnyc ferdnyc commented Apr 4, 2024

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

Fixes #1781

@ferdnyc
Copy link
Member Author

ferdnyc commented Apr 4, 2024

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
Copy link
Member Author

ferdnyc commented Apr 4, 2024

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 ferdnyc marked this pull request as draft April 4, 2024 11:46
@ferdnyc
Copy link
Member Author

ferdnyc commented Apr 4, 2024

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
Copy link
Collaborator

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
Copy link
Member Author

ferdnyc commented Apr 4, 2024

@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
Copy link
Collaborator

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

Copy link
Member Author

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm glad I did that, because I discovered that the image is missing dbus-devel, oddly enough — guess it's not pulled in as a dependency anymore. So I'll have to fix the Dockerfile over there.

But the extension is broken, specifically, because of this boneheaded code.

src/service/backends/lan.js Outdated Show resolved Hide resolved
@ferdnyc ferdnyc marked this pull request as ready for review April 5, 2024 02:10
@ferdnyc
Copy link
Member Author

ferdnyc commented Apr 5, 2024

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
Copy link
Collaborator

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

Copy link
Member

@daniellandau daniellandau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I didn't have time to test locally and didn't quite follow the logic of the hesitation discussion but let's get this in before the release if there's no pressing reason not to.

@ferdnyc
Copy link
Member Author

ferdnyc commented Apr 6, 2024

@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
Copy link
Collaborator

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

@andyholmes
Copy link
Collaborator

andyholmes commented Apr 6, 2024

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
Copy link
Member Author

ferdnyc commented Apr 6, 2024

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
Copy link
Collaborator

Ah, disappointing :(

@retrixe
Copy link
Contributor

retrixe commented Apr 6, 2024

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
Copy link
Member Author

ferdnyc commented Apr 6, 2024

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
Copy link
Member Author

ferdnyc commented Apr 6, 2024

@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
Copy link
Collaborator

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 ferdnyc merged commit ad2cf7f into GSConnect:main Apr 8, 2024
3 checks passed
@ferdnyc ferdnyc deleted the gio-unix branch April 8, 2024 19:32
@ferdnyc
Copy link
Member Author

ferdnyc commented Apr 8, 2024

Sorry, was asleep at the wheel. Trigger pulled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nightly noncritical error on gnome 46
4 participants