Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 2122922 - vte2.91/gtk4 breaks upstream's ABI
Summary: vte2.91/gtk4 breaks upstream's ABI
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: vte291
Version: 37
Hardware: Unspecified
OS: Linux
unspecified
high
Target Milestone: ---
Assignee: David King
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-08-31 09:24 UTC by Christian Persch
Modified: 2022-11-08 01:49 UTC (History)
5 users (show)

Fixed In Version: vte291-0.70.1-1.fc37
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-11-08 01:49:12 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Christian Persch 2022-08-31 09:24:01 UTC
The fedora patches applied altogether cause this difference to vteterminal.h:
```
        void (*bell)(VteTerminal* terminal);
 
-#if _VTE_GTK == 3
-        /* Compatibility padding due to fedora patches intruding on our ABI */
-        /*< private >*/
-        gpointer _extra_padding[3];
-#endif /* _VTE_GTK == 3 */
+       void (*notification_received)(VteTerminal* terminal, const gchar *summary, const gchar *body);
+       void (*shell_precmd)(VteTerminal* terminal);
+       void (*shell_preexec)(VteTerminal* terminal);
 
         /* Add new vfuncs here, and subtract from the padding below. */
 
         /* Padding for future expansion. */
 #if _VTE_GTK == 3
         gpointer _padding[13];
 #elif _VTE_GTK == 4
         gpointer _padding[16];
 #endif /* _VTE_GTK */
```

This leads to the struct size on vte/gtk4 being 3 pointers larger than upstream's since you don't decrease the padding for gtk4.

As the (removed by the patch) comment inside the `#if _VTE_GTK == 3` block says, it exists purely to accomodate the fedora patches on vte/gtk3 since it's way too late to change anything there.  For vte/gtk3, the patch adds the vfuncs in the right place, but you should *keep* the `#if` around them. 

For vte/gtk4, which has never seen a stable release yet and not been in fedora before, the best way would of course to drop the patches (i.e. make them vte/gtk3 only and add the appropriate `#if VTE_GTK == 3` around its implementation in the `.cc` files).  However, if you do want to add them to vte/gtk4 also, please do not intrude on the upstream ABI and API by using API names and object property names that vte will want to use for itself. Instead, you should:

* Add new vfuncs for vte/gtk4 after the `_padding[16]` and subtract from that padding.  Add them *after* since vte itself will add new vfuncs at the *start* under the `
/* Add new vfuncs here, and subtract from the padding below. */` comment.  Putting yours after will keep them from conflicting (unless vte upstream adds more than 13 new vfuncs before the next upstream ABI/API break, which is unlikely). Surround new vfuncs added there with a `#if _VTE_GTK == 4` ... `#endif` block.

* Alternatively, add no new vfuncs and make the new GObject signals not have vfuncs at all.

* For added API, add a `fedora` prefix or suffix, e.g. call fedora-specific added API `vte_terminal_set_scroll_speed_fedora` instead of just ``vte_terminal_set_scroll_speed`, etc.

* Same for added vfuncs.

* Same for GObject properties, e.g. `"notification-received-fedora"` etc.

Comment 1 Christian Persch 2022-09-21 08:43:41 UTC
Still the same in vte 0.70.0-1 pkg.

Please do not release F37 with this ABI break.

Comment 2 Kalev Lember 2022-09-21 09:28:10 UTC
David, can you take a look at this, please? Should be a trivial adjustment to the downstream patch. I can help coordinate the rebuilds - we have a ~ two week window to get this done before the final freeze starts.

Comment 3 Kalev Lember 2022-09-27 14:37:48 UTC
https://src.fedoraproject.org/rpms/vte291/pull-request/5 should fix this -- Christian, can you take a look that it looks good to you with this change?

Comment 4 Christian Persch 2022-09-27 19:43:12 UTC
Thanks for taking care of this. And I think it's the right choice, at least for now, to confine the fedora patch to vte/gtk3 since afaik the only user of it in fedora is gnome-terminal which is also still gtk3.

Looking at the full diff (not the individual commits), I noticed this:

```
 #if _VTE_GTK == 3
+       void (*notification_received)(VteTerminal* terminal, const gchar *summary, const gchar *body);
+       void (*shell_precmd)(VteTerminal* terminal);
+       void (*shell_preexec)(VteTerminal* terminal);
+
         /* Compatibility padding due to fedora patches intruding on our ABI */
         /*< private >*/
-        gpointer _extra_padding[3];
+        gpointer _extra_padding[1];
 #endif /* _VTE_GTK == 3 */
 ```

You add 3 vfuncs, but only subtract 2 padding?

The new functions vte_terminal_get_current_container_name and vte_terminal_get_current_container_runtime in src/vte/vteterminal.h should also be surrounded with #if _VTE_GTK == 3 .. #endif.

Comment 5 Debarshi Ray 2022-10-06 17:36:55 UTC
(In reply to Christian Persch from comment #4)
> Thanks for taking care of this. And I think it's the right choice, at least
> for now, to confine the fedora patch to vte/gtk3 since afaik the only user
> of it in fedora is gnome-terminal which is also still gtk3.
> 
> Looking at the full diff (not the individual commits), I noticed this:
> 
> ```
>  #if _VTE_GTK == 3
> +       void (*notification_received)(VteTerminal* terminal, const gchar
> *summary, const gchar *body);
> +       void (*shell_precmd)(VteTerminal* terminal);
> +       void (*shell_preexec)(VteTerminal* terminal);
> +
>          /* Compatibility padding due to fedora patches intruding on our ABI
> */
>          /*< private >*/
> -        gpointer _extra_padding[3];
> +        gpointer _extra_padding[1];
>  #endif /* _VTE_GTK == 3 */
>  ```
> 
> You add 3 vfuncs, but only subtract 2 padding?

Umm... are you reading the changes at https://src.fedoraproject.org/rpms/vte291/pull-request/5 correctly?

With David's changes, here are the changes to vteterminal.h in Fedora's fork that I see:

--- vte-0.70.0.orig/src/vte/vteterminal.h	2022-10-06 19:30:44.407238413 +0200
+++ vte-0.70.0/src/vte/vteterminal.h	2022-10-06 19:30:55.584434993 +0200
@@ -109,9 +109,9 @@ struct _VteTerminalClass {
 	void (*bell)(VteTerminal* terminal);
 
 #if _VTE_GTK == 3
-        /* Compatibility padding due to fedora patches intruding on our ABI */
-        /*< private >*/
-        gpointer _extra_padding[3];
+	void (*notification_received)(VteTerminal* terminal, const gchar *summary, const gchar *body);
+	void (*shell_precmd)(VteTerminal* terminal);
+	void (*shell_preexec)(VteTerminal* terminal);
 #endif /* _VTE_GTK == 3 */
 
         /* Add new vfuncs here, and subtract from the padding below. */
@@ -328,6 +328,12 @@ void vte_terminal_set_cursor_shape(VteTe
 _VTE_PUBLIC
 VteCursorShape vte_terminal_get_cursor_shape(VteTerminal *terminal) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);
 
+#if _VTE_GTK == 3
+_VTE_PUBLIC
+void vte_terminal_set_scroll_speed(VteTerminal *terminal,
+                                   guint scroll_speed) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);
+#endif
+
 /* Set the number of scrollback lines, above or at an internal minimum. */
 _VTE_PUBLIC
 void vte_terminal_set_scrollback_lines(VteTerminal *terminal,
@@ -546,6 +552,10 @@ glong vte_terminal_get_column_count(VteT
 _VTE_PUBLIC
 const char *vte_terminal_get_window_title(VteTerminal *terminal) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);
 _VTE_PUBLIC
+const char *vte_terminal_get_current_container_name(VteTerminal *terminal) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);
+_VTE_PUBLIC
+const char *vte_terminal_get_current_container_runtime(VteTerminal *terminal) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);
+_VTE_PUBLIC
 const char *vte_terminal_get_current_directory_uri(VteTerminal *terminal) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);
 _VTE_PUBLIC
 const char *vte_terminal_get_current_file_uri(VteTerminal *terminal) _VTE_CXX_NOEXCEPT _VTE_GNUC_NONNULL(1);

> The new functions vte_terminal_get_current_container_name and
> vte_terminal_get_current_container_runtime in src/vte/vteterminal.h should
> also be surrounded with #if _VTE_GTK == 3 .. #endif.

Good catch!

Comment 6 Christian Persch 2022-10-06 17:52:58 UTC
It's possible I misread the diff... I wasn't looking at https://src.fedoraproject.org/rpms/vte291/pull-request/5 at all since it's a diff of a diff and thus unreadable, and git-am'ing it onto fedora's vte291 packaging failed. Instead, I looked at the total diff of https://gitlab.gnome.org/davidk/vte/-/commits/vte-0-70-cntnr-precmd-preexec-scroll (which I had assumed to be the origin of the patch) to upstream's vte-0-70 branch.

Comment 7 Matthias Clasen 2022-10-06 18:45:03 UTC
Whats the actual abi concern?

Do you expect to swap out Fedora vte builds with 'vanilla' vte builds? Did we promise somewhere that that is possible?

Comment 8 Christian Persch 2022-10-06 19:10:37 UTC
Yes, I expect that fedora doesn't break my upstream ABI, and esp. not to make changes that are so hugely incompatible as enlarging a public struct, or adding public vfuncs in places where I'm going to add my own new vfuncs upstream next. vte/gtk3 is a lost cause in that regard, but vte/gtk4 is entirely new and hasn't been in fedora before.

No, you didn't promise not to, but I'm *asking* you not to.

Also note that patching vte/gtk4 is entirely unnecessary, since afaik the only consumer of the patch's functionality is gnome-terminal which is still gtk3 only, and which fedora is going to replace with gnome-console in the near future (https://pagure.io/fedora-workstation/issue/261).

Comment 9 Fedora Update System 2022-10-17 08:29:34 UTC
FEDORA-2022-07ceffb257 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2022-07ceffb257

Comment 10 Fedora Update System 2022-10-18 12:41:05 UTC
FEDORA-2022-07ceffb257 has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-07ceffb257`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-07ceffb257

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 11 Fedora Update System 2022-10-30 19:22:16 UTC
FEDORA-2022-ce3b8fbd4d has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-ce3b8fbd4d`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-ce3b8fbd4d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 12 Fedora Update System 2022-11-01 14:47:02 UTC
FEDORA-2022-ce3b8fbd4d has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-ce3b8fbd4d`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-ce3b8fbd4d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 13 Fedora Update System 2022-11-03 08:46:35 UTC
FEDORA-2022-ce3b8fbd4d has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-ce3b8fbd4d`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-ce3b8fbd4d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 14 Fedora Update System 2022-11-08 01:49:12 UTC
FEDORA-2022-ce3b8fbd4d has been pushed to the Fedora 37 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.