Bug 22137 - 1.8.0 Cairo::Matrix API/ABI breakage
Summary: 1.8.0 Cairo::Matrix API/ABI breakage
Status: RESOLVED FIXED
Alias: None
Product: cairomm
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Murray Cumming
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-07 09:00 UTC by Jānis Rukšāns
Modified: 2010-06-07 06:06 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Revert "scaled_matrix() -> scaling_matrix()" (1.34 KB, patch)
2009-06-08 20:35 UTC, Jonathon Jongsma
Details | Splinter Review
Revert "Add an overloaded Context::get_matrix() that returns a copy of the Matrix" (2.22 KB, patch)
2009-06-08 20:35 UTC, Jonathon Jongsma
Details | Splinter Review
Revert "Fix broken Pattern::get/set_matrix() API that was using the C types" (2.34 KB, patch)
2009-06-08 20:36 UTC, Jonathon Jongsma
Details | Splinter Review
Revert "Overhaul of the newly-wrapped Matrix API (+documentation)" (12.95 KB, patch)
2009-06-08 20:36 UTC, Jonathon Jongsma
Details | Splinter Review
Revert "Inherit Cairo::Matrix from cairo_matrix_t" (24.86 KB, patch)
2009-06-08 20:36 UTC, Jonathon Jongsma
Details | Splinter Review
Fix test breakage from reverting Matrix changes (15.66 KB, patch)
2009-06-08 20:36 UTC, Jonathon Jongsma
Details | Splinter Review
Add overloads taking (const) cairo_matrix_t & to keep ABI (8.50 KB, patch)
2009-06-09 02:28 UTC, Jānis Rukšāns
Details | Splinter Review

Description Jānis Rukšāns 2009-06-07 09:00:57 UTC
* Cairo::Matrix now inherits from cairo_matrix_t
  * Previously, we had used Cairo::Matrix throughout our API, but
    Cairo::Matrix was just a typedef for cairo_matrix_t

This breaks cairomm API/ABI. For example, BMPx (built against 1.6.x, uses cairomm to display "libnotify-like" popup for tray icon) crashes with

/usr/libexec/beep-media-player-2-bin: undefined symbol: _ZN5Cairo7Context10set_matrixERK13_cairo_matrix


In a way, API is also broken and rebuilding doesn't help:

popup.cc: In member function 'void Bmp::Popup::draw_arrow(Cairo::RefPtr<Cairo::Context>&, int, int)':
popup.cc:311: error: braces around initializer for non-aggregate type 'Cairo::Matrix'

The offending line is

Cairo::Matrix matrix = { 1, 0, 0, -1, 0, h };
Comment 1 Jonathon Jongsma 2009-06-07 13:04:57 UTC
This is extremely unfortunate.  I seem to have really screwed this up.  I guess I'll have to revert all of those matrix changes.  I naively thought they would be OK for API/ABI...
Comment 2 Murray Cumming 2009-06-07 13:44:16 UTC
I think you should first establish what you thought would happen and what has actually happened. Maybe you can then make only a limited change to fix it.

We maybe also need to consider if this new ABI has already become established.
Comment 3 Jānis Rukšāns 2009-06-08 00:17:08 UTC
What has happened is the mangled function names were changed.

For functions that take reference to Cairo::Matrix as argument, overloads taking a reference to cairo_matrix_t should fix forward ABI compatibility. Backwards compatibility would still be broken and code requiring 1.6.x but built with 1.8.x headers won't run with 1.6.x at runtime. ATM I can't think of anything that could fix the latter.
Comment 4 Murray Cumming 2009-06-08 00:27:24 UTC
Can't we just add back a Context::set_matrix() overload that takes cairo_matrix_t?
Comment 5 Jānis Rukšāns 2009-06-08 00:42:59 UTC
Well you need overloads for every function that takes Cairo::Matrix. And if you don't care about code compiled with 1.8.x headers but running with 1.6.x then that should be it (sorry I messed up with backward/forward compatibility).
Comment 6 Jonathon Jongsma 2009-06-08 20:35:57 UTC
Created attachment 26572 [details] [review]
Revert "scaled_matrix() -> scaling_matrix()"

This reverts commit 87a5ddbd0f2709bdeb1d12634a37ad762adc44ab.

Conflicts:

	ChangeLog
Comment 7 Jonathon Jongsma 2009-06-08 20:35:59 UTC
Created attachment 26573 [details] [review]
Revert "Add an overloaded Context::get_matrix() that returns a copy of the Matrix"

This reverts commit ccb381d30a1a523896d2b60204c0c2588fe25f02.
Comment 8 Jonathon Jongsma 2009-06-08 20:36:00 UTC
Created attachment 26574 [details] [review]
Revert "Fix broken Pattern::get/set_matrix() API that was using the C types"

This reverts commit bbf5159397aac3f952d0c17a5e66e7fc40177b8b.

Conflicts:

	ChangeLog
	cairomm/pattern.h
Comment 9 Jonathon Jongsma 2009-06-08 20:36:02 UTC
Created attachment 26575 [details] [review]
Revert "Overhaul of the newly-wrapped Matrix API (+documentation)"

This reverts commit dc26aaf781da09b69598cf106ec79c8c06fca8fa.

Conflicts:

	.gitignore
	ChangeLog
	cairomm/matrix.h
	tests/test-font-face.cc
	tests/test-matrix.cc
Comment 10 Jonathon Jongsma 2009-06-08 20:36:03 UTC
Created attachment 26576 [details] [review]
Revert "Inherit Cairo::Matrix from cairo_matrix_t"

This reverts commit c0fab1927cab33f35d43b421c3464351bb8d8072.

Conflicts:

	cairomm/matrix.h
	cairomm/types.h
	tests/Makefile.am
	tests/test-font-face.cc
	tests/test-matrix.cc
Comment 11 Jonathon Jongsma 2009-06-08 20:36:05 UTC
Created attachment 26577 [details] [review]
Fix test breakage from reverting Matrix changes
Comment 12 Jonathon Jongsma 2009-06-08 20:38:26 UTC
I've attached a set of patches that revert all of the matrix changes that caused this whole mess.  Should we push these and declare 1.8.0 unusably broken and make a new release?
Comment 13 Murray Cumming 2009-06-09 01:58:48 UTC
I really think you should consider if both ABIs can be supported.
Comment 14 Jānis Rukšāns 2009-06-09 02:23:04 UTC
I agree with Murray. I patched and rebuilt F10's cairomm-1.8.0 RPMs with overloads taking (const) cairo_matrix_t & and BMPx runs fine. Is there anything else that I could test that uses other functions than Context::set_matrix?
Comment 15 Jānis Rukšāns 2009-06-09 02:28:09 UTC
Created attachment 26580 [details] [review]
Add overloads taking (const) cairo_matrix_t & to keep ABI

I added the overloads only to pre-1.8.0 functions. They're public but I think they might as well be private.
Comment 16 Jonathon Jongsma 2010-06-07 06:06:29 UTC
hm,  I seem to have forgotten to close this bug.  I had committed patch #26580 quite some time ago, so it has been fixed since release 1.8.2.  Thanks for the patch, and sorry for the lack of feedback.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.