McEs, A Hacker Life: JDS Patch Review

Tuesday, June 19, 2007

JDS Patch Review

(Note: After receiving two comments and rereading my previous post, I removed it in accordance to CoC which I have volunteerly signed. Well, we're cool now.)

Seeing people commenting on JDS patches for their packages, I couldn't help but checking pango, cairo, and vte patches they ship. Having reviewed them, I thought I should provide some feedback :). I'm posting here, hoping that other GNOME hackers pick this up too. Later we can move on to collectively review other vendors' patches too, and help the over-busy maintainers push the right patches upstream and drop/fix the wrong ones. Not every project has a sebuild, right? ;) Brian Cameron has been doing a great job doing that for opensolaris already, but there's more room.

pango-01-fullwidth-space.diff: I'm interested to have a Pango bug requesting what this tries to fix, as I'm not sure about what the patch tries to achieve. Other than that, both changes in the patch are wrong. The first one is adding an entry for 0x0020 after 0x2000 in a table that is supposed to remain sorted. Moreover, characters below 0x2000 are not looked up in that table at all, that's why the table starts at 0x2000. The other change too adds an entry, for 0x3000, but does not remove the old entry for 0x3000. Both can confuse the bsearch routine.
pango-02-pua.diff: Is a very weird attempt that I believe is trying to fix bug 443206 that was fixed upstream two weekes ago. Other than that, the patch touches public pango API, something that should be avoided at all costs. It adds a new enum value PANGO_SCRIPT_OTHER that makes no sense. Just use the upstream patch please, or wait for next stable release.
pango-03-no-xrender.diff: Removes check for xrender from pango's configure check for pangoxft. If that check is not necessary, and there's a use in compiling without it, please file a bug agaist Pango and I'll be more than happy to commit it upstream.
cairo-01-uninstalled-pc.diff: Submitted to upstream bugzilla already. Only if someone could tell me how exactly the -uninstalled pkg-config files are used...
cairo-02-8bit-fix.diff: This works around the infamous cairo 8-bit issues. The patch has been posted upstream. Carl has been working on getting this fixed, but it's not easy, and a clean fix is even harder.
cairo-03-full-hinting.diff: I'm interested to hear what the patch tries to achieve. I assume this relies on custom changes to fontconfig.
cairo-04-mediaLib.diff: In the works in upstream bugzilla already.
cairo-05-remove-buggy_repeat.diff: Committed upstream already.
vte-01-fcconfig.diff: According to Owen, patch is clearly wrong, and I agree. The issue is more of a fontconfig misconfiguration IMO, but not enough detail is given, so I can't test. Needs someone debugging what's going on, instead of blindly cut it out.
vte-03-cut-copy-paste-handle.diff: The patch looks interesting. I don't remember seeing any upstream report about it, and I'm not sure what problem it tries to solve, but I like to know!
vte-04-selection-perf-improve.diff: Well, this was submitted upstream and rejected. It removes vte's regular expression matching functionality and hardcodes hand-written matching of common URL schemes. There are many other ways to improve vte's selection performance without sacrificing functionality. Needs someone to profile/debug it.

All in all, very good for cairo, but can do better for vte and pango.

Labels: cairo, jds, pango, vte

¶ 5:46 PM

Comments:

The -uninstalled files were invented for "gtk+-enterprise-edition" which was a big tarball containing gtk and all dependencies created for reasons that I don't really remember.

To build the tarball, you had to be able to run make from the toplevel and build all the subdirectories (glib, pango, gtk, etc.), without a make install on each dependency as you went.

It was very hard to maintain a build that worked right in this giant tarball context, but the uninstalled .pc files were part of what you had to do.

# posted by

Havoc : June 19, 2007 7:24 PM

http://live.gnome.org/VendorPatches has links to a whole bunch of vendor patches.

# posted by

Anonymous : June 20, 2007 1:08 AM

I'll take a bow any day for a man that is capable of admitting he was wrong, so my hat's off to you Behdad!

And a good thing to see you turned that energy elsewhere and reviewed patches like a madman instead ;)

Keep up the good work!

/A humble admirer

# posted by

Anonymous : June 20, 2007 2:20 AM

In Sun we use uninstalled pc because we often group multiple dependant modules in one package and don't install the package until all modules are built.
For example, if SUNWmeta-pkg has modules A, B and C.
The build sequence is:
* A.prep
* B.prep
* C.prep
* A.build (configure; make)
* B.build
* C.build
* A.install (make install to $RPM_BUILD_ROOT)
* B.install
* C.install
* Generate package.
* Install package

If module B or C needs uses pkg-config to reference module A, an uninstalled.pc file is used.

Does this help explain why patches like cairo-01-uninstalled-pc.diff exist?

# posted by

Anonymous : June 20, 2007 5:13 AM

Feel free to review the patches in the pango1.0 and vte packages maintained by the Debian GNOME Team! :)

Currently, the latest versions of these can be found at:
desktop/experimental/pango1.0/debian/patches
desktop/unstable/vte/debian/patches

I've updated the VendorPatches page with more complete and recent information.

We're in the process of slowly integrating bug references and descriptions at the top of patches, but not all patches carry such information yet.

# posted by

lool : June 20, 2007 5:21 AM

vte-03-cut-copy-paste-handle.diff says "Bind Copy, Paste, Cut keys". It seems to bind F16, F18, and F20 (GDK_F16, etc.) keys to signals "copy-clipboard" and "paste-clipboard".

I guess "cut" makes little sense for a terminal widget (that's what VTE is, right?), so the key marked "cut" generates "copy", as well.

Sun people have always had weird keyboards. :-)

# posted by

Anonymous : June 20, 2007 4:12 PM

This is Brian Cameron (Brian.Cameron@sun.com). Thanks for the nice comment.

Here is some feedback and bugzilla bug numbers.

The pango-01-fullspace.diff patch is in bugzilla bug 145275. Note the older patch which is sort of like the patch we apply here at Sun. If there's a better way to fix, this then let me know and I'll be happy to update our patch.

The pango-03-no-xrender.diff patch is because the Sun Xserver team doesn't yet ship the pc files. They are slowly fixing this, and we'll remove this patch once the pc file is in Solaris.

I think that the cairo uninstalled pc patch has been discussed well enough. Most GNOME modules that provide libraries provide this file to make building possible when dependent modules are not yet installed. Our build process here at Sun depends on this feature.

The vte-01-fcconfig.diff is bugzilla 142640. vte-03-cut-copy-paste.diff is bugzilla 153265. vte-04-selection-perf-improve is bugzilla 143914.

# posted by

Anonymous : June 25, 2007 4:53 AM

About Me

Twitter Updates