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

unintentional fwupd_client_connect API change between 1.6.1 and 1.6.2? #3644

Closed
decathorpe opened this issue Aug 16, 2021 · 5 comments
Closed
Labels

Comments

@decathorpe
Copy link

decathorpe commented Aug 16, 2021

Describe the bug

One of my Fedora packages fails to compile since the fwupd package was updated from version 1.6.1 to 1.6.2. I was not sure whether this is caused by an upstream issue or not, so I opened a ticket with switchboard-plug-about first:

elementary/switchboard-plug-about#224

This is the error message:

../src/Interfaces/FirmwareClient.vala:31.17-31.33: error: 2 missing arguments for `unowned GLib.Object GLib.Object.connect (string, ...)'
                client.connect ();
                ^^^^^^^^^^^^^^^^^

Steps to Reproduce

Digging a bit deeper (running diffoscope between fwupd-devel 1.6.1 and fwupd-devel 1.6.2), it looks like there was an unintended (?) vala API change between those versions, namely, the Fwupd.Client.connect method was removed:

 		public bool clear_results (string device_id, GLib.Cancellable? cancellable = null) throws GLib.Error;
 		[Version (since = "1.5.0")]
 		public async bool clear_results_async (string device_id, GLib.Cancellable? cancellable) throws GLib.Error;
-		[Version (since = "0.7.1")]
-		public bool connect (GLib.Cancellable? cancellable = null) throws GLib.Error;
 		[Version (since = "1.5.0")]
 		public async bool connect_async (GLib.Cancellable? cancellable) throws GLib.Error;
 		[Version (since = "1.4.5")]

This seems to have been an intentional change:
08caad4
Which was part of a change to provide async methods in addition to sync methods, IIUC.

(Whether switchboard-plug-about should use the new async methods in the future is probably a different issue - but ubuntu has a different update policy there, so I don't know if they could even start relying on fwupd 1.6.2+ right now.)

Expected behavior

I would expect that "patch" version bumps should not change public APIs. Though it's possible that I just don't understand Vala / GObject well enough to understand whether this is an acceptable change or not. The commit messages related to the changes causing my issue seem to indicate that this is fine.

fwupd version information

With fwupd versions until 1.6.1, my package compiled fine. With version 1.6.2, it is broken.

  • Operating system and version: Fedora 34+
  • Have you tried rebooting? N/A
  • Is this a regression? Yes
@decathorpe decathorpe added the bug label Aug 16, 2021
@decathorpe decathorpe changed the title unintentional fwupd_client_connet API change between 1.6.1 and 1.6.2? unintentional fwupd_client_connect API change between 1.6.1 and 1.6.2? Aug 16, 2021
@superm1
Copy link
Member

superm1 commented Aug 16, 2021

Would it be possible to compile 1.6.3 with just that reverted to confirm that helps? I think it will.
I'm leaning on what we should probably do is revert it for 1.6.x and then push that change into 1.7.x (where we are actually having an ABI change).

@superm1
Copy link
Member

superm1 commented Aug 16, 2021

The other thing to mention - IIRC you don't need to explicitly call connect. It's implicitly called from other functions. So making a code change to not call it should help make your application work with all fwupd versions too, whether or not this API change happens.

@hughsie
Copy link
Member

hughsie commented Aug 16, 2021

We dropped the explicit FwupdClient.connect() as it was preventing using the GObject.connect to connect up signals as we were overriding the method. I think "connect" for an unrelated thing was a really bad choice all those years ago. I think the solution we should do is:

  • Somehow rename fwupd_client_connect() to something like fwup_client_connect_daemon() in the introspection (and thus the vala) -- maybe it makes sense to rename/add the C symbol too.
  • Use a different sync method which calls fwupd_client_connect() as a side-effect, e.g. FwupdClient.get_devices()

@decathorpe
Copy link
Author

decathorpe commented Aug 16, 2021

Oh, so dropping the explicit .connect() method caused the "overridden" / underlying GObject connect method - which has a different API and different semantics - to surface ... yeah that's unfortunate. I removed the Fwupd.Client.connect call from the code and things still seem to work (at least it compiles, there's no logged error messages, UEFI dbx still shows up as a "device", etc.).

@decathorpe
Copy link
Author

decathorpe commented Aug 24, 2021

switchboard-plug-about has removed this API call, as it turned out it wasn't even necessary to call .connect() explicitly here. While this doesn't really "fix" the "API change", I'd call this issue resolved (at least for me). Thanks for your help!

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

No branches or pull requests

3 participants