Bug 2122922
| Summary: | vte2.91/gtk4 breaks upstream's ABI | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Christian Persch <chpe> |
| Component: | vte291 | Assignee: | David King <amigadave> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | high | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | 37 | CC: | amigadave, debarshir, gnome-sig, klember, mclasen |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | Unspecified | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | vte291-0.70.1-1.fc37 | Doc Type: | If docs needed, set a value |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2022-11-08 01:49:12 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
Still the same in vte 0.70.0-1 pkg. Please do not release F37 with this ABI break. 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. 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? 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.
(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! 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. 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? 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). FEDORA-2022-07ceffb257 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2022-07ceffb257 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. 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. 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. 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. 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. |
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.