Created attachment 301943 [details] [review]
scanner: honor XDG_CACHE_HOME

We already use XDG_DATA_DIRS for .gir files lookup
so we might as well honor XDG_CACHE_HOME instead
of hardcoding ~/.cache.

Review of attachment 301943 [details] [review]:

Looks fine, though I would add a comment like "This is Python reimplementation of `g_get_user_cache_dir()` because g-i can't depend on GLib via introspection; if any changes are made to that function they'll need to be copied here".

Created attachment 302024 [details] [review]
scanner: add "XDG Base Directory" functions

(In reply to Colin Walters from comment #2)
> Looks fine, though I would add a comment like "This is Python
> reimplementation of `g_get_user_cache_dir()` because g-i can't depend on
> GLib via introspection; if any changes are made to that function they'll
> need to be copied here".

This applies on top of attachment #304319 [details] and extracts the cache
and data dir lookup code into documented get_user_cache_dir() and
get_system_data_dirs() functions.

Note that previously, the data dirs code did not fall
back to '/usr/local/share:/usr/share' when the XDG_DATA_DIRS
environment variable was either not set or empty, as required
by the XDG Base Directory Specification. Not sure if this
was just forgotten or an intentional omission...

Created attachment 302025 [details] [review]
scanner: add "XDG Base Directory" functions

ahum, now with correct data dir fallback order...

Nope, the patch from comment #3 was actually correct. Sorry for the noise :/

Review of attachment 302024 [details] [review]:

::: giscanner/transformer.py
@@ +179,3 @@
+        data_dirs.append(DATADIR)
+        if os.name != 'nt':
+            data_dirs.append('/usr/share')

Should get_system_data_dirs() already contain /usr/share? You're adding it twice. In case of an override from some environment-tweaking script like jhbuild, then you probably still want to add /usr/local/share as well.

::: giscanner/utils.py
@@ +226,3 @@
+    if homedir is not None and os.path.exists(homedir):
+        cachedir = os.path.join(homedir, '.cache')
+    '''

Isn't this a bit of a race?

The correct way would be to try and create the directory, and if it fails because it exists already, then handle that gracefully.

Created attachment 304891 [details] [review]
scanner: add "XDG Base Directory" functions

(In reply to Emmanuele Bassi (:ebassi) from comment #6)
> Review of attachment 302024 [details] [review] [review]:
> 
> ::: giscanner/transformer.py
> @@ +179,3 @@
> +        data_dirs.append(DATADIR)
> +        if os.name != 'nt':
> +            data_dirs.append('/usr/share')
> 
> Should get_system_data_dirs() already contain /usr/share? You're adding it
> twice. In case of an override from some environment-tweaking script like
> jhbuild, then you probably still want to add /usr/local/share as well.

The original code this this, so I kept it around in an overabundance of carefulness.
Added a comment to that effect.
 
> ::: giscanner/utils.py
> @@ +226,3 @@
> +    if homedir is not None and os.path.exists(homedir):
> +        cachedir = os.path.join(homedir, '.cache')
> +    '''
> 
> Isn't this a bit of a race?
> 
> The correct way would be to try and create the directory, and if it fails
> because it exists already, then handle that gracefully.

Reworked a bit to address your concern.

Created attachment 305714 [details] [review]
scanner: add "XDG Base Directory" functions

Extract cache and data dir lookup code into documented
get_user_cache_dir() and get_system_data_dirs() functions.

Note that previously, the data dirs code did not fall
back to '/usr/local/share:/usr/share' when the XDG_DATA_DIRS
environment variable was either not set or empty, as required
by the XDG Base Directory Specification.

Review of attachment 305714 [details] [review]:

Looks OK to me.

(In reply to Colin Walters from comment #9)
> Review of attachment 305714 [details] [review] [review]:
> 
> Looks OK to me.

Both patches pushed. Thanks for the review Emmanuele and Colin.