GNOME Bugzilla – Bug 774491
location of babl fish cache reconsideration on OSX
Last modified: 2018-05-22 12:15:31 UTC
Hi all! I've been compiling gimp, BABL and GEGL on my windows machine for a while now, and some recent changes to babl-cache caused some errors and now BABL won't build. here is the log: CC babl-fish-reference.lo babl-cache.c: In function 'fish_cache_path': babl-cache.c:47:5: error: too many arguments to function 'mkdir' mkdir (resolved, S_IRWXU); ^~~~~ In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/sys/stat.h:14 :0, from babl-cache.c:20: C:/msys64/mingw64/x86_64-w64-mingw32/include/io.h:280:15: note: declared here int __cdecl mkdir (const char *) __MINGW_ATTRIB_DEPRECATED_MSVC2005; ^~~~~ CC babl-fish-simple.lo Specs: - Windows 10 Pro 64-bit - MINGW64 via msys2 - gcc --version gcc.exe (Rev2, Built by MSYS2 project) 6.2.0 - Latest updates from official repo. - Self compiled GEGL, BABL and Gimp.
The commit that caused the problem appears to be this one: https://git.gnome.org/browse/babl/commit/?id=d8259096e7e78e768147301a3d5ef8e8afed7a99
This might fix the issue with compilation, win32 will continue being a second or third grade citizen until someone contributes patches implementing a location for the babl fish cache - and code for making it exist. commit 4fcf3fe10e8addfc7f39b7037a1d468abdf29a68 Author: Øyvind Kolås <pippin@gimp.org> Date: Tue Nov 15 23:51:44 2016 +0100 babl-cache: attempt to make things barely compile on win32
Created attachment 340320 [details] [review] Initial solution for babl cache on win32, plus misc bugfixes I'm using MSYS2/mingw32 on XP SP3. With babl cache working, GIMP starts in 10 seconds... on my Celeron M! So, after some generous fprintf(stderr): On Windows, babl was regenerating the cache every time, because the cache version test always failed due to a trailing '\r'. Evil CRLF. As for where to keep the cache, the best option for now I think is just to use $TEMP. Worse case, GIMP takes longer to start if TEMP is emptied. For further discussion: 1. Hard-coded paths are a bad idea, since e.g. "C:\Users\USER\AppData\Local" (or "C:\Documents and Settings\USER\Local Settings\Application Data" on XP) could be relocated or in a different language. 2. Should the cache go in user roaming storage (where GIMP-2.9 puts brushes and stuff) or user local storage (which stays with the machine - TEMP usually goes here)? 3. Also should we read environment variables (APPDATA etc.), or make API calls (used by GIMP, recommended by MS to guarantee correctness)? 4. Am I overthinking this? ;) See also: https://technet.microsoft.com/en-us/library/cc766489(WS.10).aspx http://superuser.com/a/775943 While looking at the source, I saw a few other things to fix. In commit 9e44467, the 1% fish discard test would drop the entire cache. Except that instead, a divide by zero would crash GIMP on startup >1% of the time. Maybe some recent bug reports from this? Also, in commit 90491e4, BABL_LEGAL_ERROR and _babl_legal_error() were copied to babl-fish-stats.c, then later only one copy of BABL_LEGAL_ERROR got changed. I've assumed this should be unified for consistency/maintainability. I'm new to contributing, and not sure of coding policy and style, so please look this patch over carefully before committing (especially my fixmes). (Does mkpath() violate LGPL, BTW?) Don't forget to "del C:\babl.txt" :)
Comment on attachment 340320 [details] [review] Initial solution for babl cache on win32, plus misc bugfixes TEMP is frequently cleaned out automatically for some people.
$TEMP was only a "temporary" solution. :) I expect pippin or someone to go over this patch with a fine-toothed comb. The reason I thought TEMP might work (for now) is it should never be read-only - the original file was in the root of C:\, where file permissions might interfere. On second thought $APPDATA might be better. I'm able to implement any of the options discussed above, I just wanted to have input on the best course of action. Each solution has unique platform issues to work around (XP vs Vista+, mainly). At least the cache works now - well for me anyway.
Please keep the number of (related) changes in one patch to a minimum. With many different bug-fixes in one big patch the patch as a whole stops applying cleanly with changes elsewhere in the code, and it is more difficult to directly incorporate individual fixes.
I think all parts of the patch have been applied to the babl repo now - though without proper attribution apart from in this commit message. Renaming and keeping bug open pending decision on path, expanding scope to include OSX. commit 1764ff583e5967622469ac687eecbaaf96c2343d Author: Øyvind Kolås <pippin@gimp.org> Date: Sun Nov 20 18:17:52 2016 +0100 babl-cache: use getenv("TEMP") for folder location on win32, and fix mk_ancestry Inspired by initial work in patch for bug 774491 provided by Edward E
> As for where to keep the cache, the best option for now I think is just to > use $TEMP. Worse case, GIMP takes longer to start if TEMP is emptied. > > For further discussion: > 1. Hard-coded paths are a bad idea, since e.g. "C:\Users\USER\AppData\Local" > (or "C:\Documents and Settings\USER\Local Settings\Application Data" on XP) > could be relocated or in a different language. > 2. Should the cache go in user roaming storage (where GIMP-2.9 puts brushes > and stuff) or user local storage (which stays with the machine - TEMP > usually goes here)? The profiling is machine specific - since various SIMD capabilities might vary between machines, relative timings of candidate paths are irrelevant though - since the cache only contains the winner of various source/destination babl format pairs. Thus "user local storage". > 3. Also should we read environment variables (APPDATA etc.), or make API > calls (used by GIMP, recommended by MS to guarantee correctness)? I think that doing the same that GIMP does - would make the experience more coherent. > 4. Am I overthinking this? ;)
Add a commit with part of the other clean-ups from the patch with many fixes, making sure to give some credit where due in the git log. commit ef962e03042c8edda42b384e5c44ef93a23721f5 Author: Edward E <develinthedetail@gmail.com> Date: Tue Nov 22 20:50:34 2016 +0100 babl-cache: Add a #define for FALLBACK_CACHE_PATH Also return fallback cache path if failing to create location for constructed path.
Created attachment 340645 [details] [review] Patch tools/babl_verify.c for portability I formatted the environment strings for visibility, hope that's OK.
Patch applied and pushed to master, thanks :)
Created attachment 341191 [details] [review] Patch: a better cache path for Win32 I agree with Michael in comment 4, TEMP is maybe too volatile for this purpose. For its plug-ins path, GEGL uses glib's g_get_user_data_dir() to retrieve the user's current local data directory, then appends GEGL_LIBRARY. Since babl doesn't use glib, this patch gets the same path using a Win32 API call, SHGetFolderPath(), then appends BABL_LIBRARY. If that path fails (it shouldn't), we use TEMP as before (which usually is set per user on Windows); then the fallback path. BTW, SHGetFolderPath() is way simpler than the API glib uses, and is available back to Windows 2000 Pro.
Patch applied, from what I hear OSX works with the same behavior as Linux; even if some slightly different paths would be more natively estethic. commit 10007b5bce8a2182100caae8fbe18078c7804009 Author: Edward E <develinthedetail@gmail.com> Date: Thu Dec 1 18:39:11 2016 -0600 babl-cache: implement a better cache path on Win32 Use SHGetFolderPathA() to get the user local data path (the same path GEGL uses for plugins on Win32) https://bugzilla.gnome.org/show_bug.cgi?id=774491
Created attachment 347766 [details] [review] fix cache path on linux on linux babl doesn't honor $XDG_CACHE_HOME variable and always set path to $HOME/.cache a patch correct this behavior
Thank you :) commit 02b2397fe72ccaa155b5ff8f240a18ea5679b477 Author: Victor Ananjevsky <ananasik@gmail.com> Date: Sun Mar 12 20:01:38 2017 +0100 fish-cache: fix cache path on linux make babl on linux honor $XDG_CACHE_HOME variable, falling back to using $HOME/.cache
Review of attachment 341191 [details] [review]: this is commited
Review of attachment 347766 [details] [review]: has been commited
Review of attachment 340320 [details] [review]: superseded by other patch
Review of attachment 340645 [details] [review]: superseced by other patch
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gegl/issues/39.