GNOME Bugzilla – Bug 688217
babl_format("cairo-ARGB32"): not found and no UI button/irresponsiveness on Windows port
Last modified: 2012-11-19 05:04:06 UTC
Created attachment 228849 [details] Broken UI on Windows 64 bits When cross-compiling the GIT repository for Windows 64 bits, the UI ends up unresponsive, with no buttons in the GIMP windows. And only solution is to kill GIMP. On the console, the only error which may be related is: ----------------------------------------------- babl_format.c:667 babl_format() babl_format("cairo-ARGB32"): not found 'grep' is not recognized as an internal or external command, operable program or batch file. ----------------------------------------------- Screenshot attached showing the problem (sorry, all in Korean. But I found the English message on other reports, like here: http://www.gimpchat.com/viewtopic.php?f=7&t=5524#p69372 Partha already has a fix for this on his experimental port. So this bug report is merely a convenience.
Created attachment 228851 [details] Batch file to start gimp 2.9 currently needs BABL_PATH set to work correctly on windows. (This should probably be fixed within GIMP at some point in the future.) Attached is a batch file that I use for my nightly builds of the gimp. It sets the required variables for you. You can use it by simply placing it in the root directory of your gimp build (outside of the bin/ folder) and then running it. Like I said, this is a bug in the gimp, so this batch file shouldn't be necessary. It's only an interim solution.
I would rather say babl should find its stuff all by itself.
babl should find its stuff all by itself, this will however have to be contributed, added and tested, by developers/users on the win32 platform.
Exactly.
So should we fill a bug against BABL then (as I can't find an existing bug report for this specific issue)?
This is a babl bug now :)
Jehan, you seem to be doing windows development, do you think you can make a babl patch to fix this?
Michael > I don't really do Windows development. I just did the other one to unblock the XDG patch. Other than this, I have not touched a Windows for years. Yet, now that I have a working Windows 7 VM and GIMP-on-Win compilation setup, also because I am a perfectionist, and finally because that looks (potentially) quite easy to handle, I may have a fast look. :-)
Created attachment 228944 [details] [review] Locating lib path with actual DLL path on Windows Hey, attached the fix. As I am very lazy, I heavily copy-pasted from GIMP code (libgimpbase/gimpenv.c, the code to get the DLL path in gimp_installation_directory()), and Glib (glib/gutf8.c for UTF16 to UTF8 conversion, because I noticed that babl does not have glib as dependency so I duplicated). Now babl will make path computation using the actual DLL path on Windows, whereas it will continue to simply use the compilation time prefix (hence expected installation path) on UNIX. The only consequence for existing UNIX implementation is that the static babl_dir_list() now returns an allocated char* (and not a constant anymore) which must be freed afterwards. That's completely unnecessary allocation/freeing on UNIX (because it either uses the return value of getenv which must not be freed or a constant string), but I thought nicer design that all version returns the same type of data. If you prefer me to make a version where babl_dir_list() still returns a const char* on UNIX with no useless allocation/free, and a char* on Windows with allocation, I can do this too.
Wow, kudos :) We can't change the babl API like that, so a const string would be preferred.
Michael > that's not part of babl API! babl_dir_list () is an internal (static) function. I think that's ok, isn't it?
Oh, of course then.
Created attachment 229022 [details] [review] New patch, without memory allocation for UNIX/Linux So I guess my patch was ok then. Anyway I still made the other version, because I think this is somehow nicer, as long as there is a good comment explaining when we have to free or not. No need to allocate memory on UNIX while we had a constant string at compile time, really. If that's ok, I think that should do it: it fixes the Win problem and does not touch the UNIX implementation at all.
Thanks for the patch, but, um, a string that has to be freed on one platform, but not on the other? I think this is not a good idea, let's go with the previous patch, unless you made functional changes in the second one.
Michael > no functional change. Let's go with the first patch then. :-) I agree that this was not the nicer idea ever. But I did not like either the fact that I had to allocate memory for a string that was a constant known at compilation time on Unix platforms. Anyway let's just use the first patch! Good for me!
Sorry for the late response, but I see that Jehan already has a fix. My solution was to set the environment variable BABL_PATH during installation. I did communicate to Mitch that I had figured out the solution.
Yes you did, but babl should default to the right path without having to set BABL_PATH. Pippin? Is this patch fine?
I am skeptical about the code lifted from glib without authorship attribution, it would probably be better to use win32 specific native APIs; if available; for the charset conversion here.
Created attachment 229310 [details] [review] Jehan's patch, minus glib functions I took Jehan's patch and removed the glib functions from it, using WideCharToMultiByte in its place. I also replaced the malloc's with babl_malloc's.
This patch has now been applied, not tested much beyond that it still compiles under linux ;) commit 4209290bc418c86dc771c66af5ffbef6ae741f41 Author: Jehan <jehan@girinstud.io> Date: Wed Nov 14 13:26:06 2012 +0900 Bug 688217 - relies on actual DLL path for locating lib directory on Windows.
Created attachment 229334 [details] [review] Cleaning out now unneeded include. Hey, sorry for glib. Considering licenses were compatible, I thought just citing in comments that the piece of code was coming from glib was ok. Next time I want to do something like this, I should "git blame" to find the exact list of authors of the functions I take? Also the last version had an unneeded include! I included stdint.h for replacing glib types into standard integers (guint32_t into uint32_t and similar type changes). But as you removed glib code, this include became unnecessary as well! Now that's too late to fix the commit as it is pushed. If we still want to clean this out, attached is a small cleaning patch.