After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 698144 - System cache for drivers and logos
System cache for drivers and logos
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
3.9
Other Linux
: Normal normal
: 3.22
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
: 691495 (view as bug list)
Depends on:
Blocks: 696727
 
 
Reported: 2013-04-16 16:17 UTC by Zeeshan Ali
Modified: 2016-03-31 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a CACHEDIR variable to the configure.ac (1.81 KB, patch)
2014-03-03 13:24 UTC, Timm Bäder
needs-work Details | Review
Add appropriate getters for system-wide logo and driver cache to utils.vala (2.42 KB, patch)
2014-03-03 13:25 UTC, Timm Bäder
needs-work Details | Review
Use the system-wide cache directory to look for logos and drivers before downloading (5.28 KB, patch)
2014-03-03 13:28 UTC, Timm Bäder
needs-work Details | Review
Fixed version of the previous patch to add a CACHEDIR variable (1.35 KB, patch)
2014-03-05 19:17 UTC, Timm Bäder
committed Details | Review
downloader: Use a string of cache dirs in download (5.22 KB, patch)
2014-03-05 19:19 UTC, Timm Bäder
needs-work Details | Review
Use the system-wide cache for logos and drivers (2.58 KB, patch)
2014-03-05 19:19 UTC, Timm Bäder
accepted-commit_now Details | Review
util: Add getters for system logo & driver cache (2.40 KB, patch)
2014-03-05 19:50 UTC, Timm Bäder
needs-work Details | Review
Use the system-wide cache for logos and drivers (2.58 KB, patch)
2014-03-05 21:59 UTC, Timm Bäder
accepted-commit_now Details | Review
downloader: Use an array of cache dirs in download (4.87 KB, patch)
2014-03-05 22:12 UTC, Timm Bäder
needs-work Details | Review
downloader: Use an array of cache dirs in download (3.97 KB, patch)
2014-03-06 08:59 UTC, Timm Bäder
needs-work Details | Review
downloader: Use an array of cache dirs in download (3.99 KB, patch)
2014-03-07 08:04 UTC, Timm Bäder
committed Details | Review
util: Add getters for system logo & driver cache (2.77 KB, patch)
2014-03-09 18:57 UTC, Timm Bäder
committed Details | Review
Use the system-wide cache for logos and drivers (2.92 KB, patch)
2014-03-09 19:07 UTC, Timm Bäder
committed Details | Review

Description Zeeshan Ali 2013-04-16 16:17:24 UTC
Currently Boxes simply attempts to download drivers and logos on-demand and then keeps them cached in users' home directory (~/.cache/gnome-box). Some distributions might want to ship these files as part of Boxes package to speed-up the first time VM creations and/or make it work offline too etc.

To allow that we should look for cached copy in /var/cache/gnome-boxes too in addition to ~/.cache/gnome-boxes before we attempt to download it.
Comment 1 Atul Jangra 2013-04-16 16:38:30 UTC
Thanks for filing the bug.
Can I get some code pointers to this bug. This would be helpful :-)
Comment 2 Zeeshan Ali 2013-04-16 20:31:23 UTC
(In reply to comment #1)
> Thanks for filing the bug.
> Can I get some code pointers to this bug. This would be helpful :-)

`git grep download` should help. i-e check all users of Downloader.download() method.
Comment 3 Atul Jangra 2013-04-16 22:39:22 UTC
If I am on right track, then what I need to do is the following:

In public async File download, we check if the file already exists in cache, i.e. ~/.cache/gnome-boxes. So in this function, I'll also check if the file exists in /var/cache/gnome-boxes. If the file exists, I'll just return the cached_file.
Now, to search for the file in /var/cache/gnome-boxes, (doubt) I can use the function get_logo_cache function in util.vala and  set var dir = "\var\cache\gnome-boxes", and return the Path.build_filename (dir, file_name).
This should tell me if I have the file as cached copy.

Few questions would be:
Is hard coding /var/cache/gnome-boxes correct way of doing this? Though logically I think this should work, but still.
Also, the way of finding the file in directory, that I've mentioned, I highly doubt it. Is this correct, and if yes, is there a better way to do this?


Thanks :-)
Comment 4 Zeeshan Ali 2013-04-17 00:02:51 UTC
(In reply to comment #3)
> If I am on right track, then what I need to do is the following:
> 
> In public async File download, we check if the file already exists in cache,
> i.e. ~/.cache/gnome-boxes. So in this function, I'll also check if the file
> exists in /var/cache/gnome-boxes. If the file exists, I'll just return the
> cached_file.

Yes but cached_path is passed to this function so you'll want to turn this into an array of paths and modify the callers of this function to pass both paths.

> Now, to search for the file in /var/cache/gnome-boxes, (doubt) I can use the
> function get_logo_cache function in util.vala and  set var dir =
> "\var\cache\gnome-boxes", and return the Path.build_filename (dir, file_name).
> This should tell me if I have the file as cached copy.

That would make it *only* search in global directory, we want to search in both. You want to add a new function there for system cache.

> Few questions would be:
> Is hard coding /var/cache/gnome-boxes correct way of doing this? Though
> logically I think this should work, but still.

No, you don't want to hardcode this. You want to make use of automake variables:

http://www.sourceware.org/autobook/autobook/autobook_106.html

You'll need to get the appropriate variable exposed to vala. While I'll leave details to you as homework (TBH I can't remember myself and find these things out each time I have to do them) but I'll give you hints: See config.vapi, AM_CPPFLAGS in Makefile.am and get_pkgdata() in util.vala.
Comment 5 Atul Jangra 2013-04-24 10:09:53 UTC
Can someone assign this bug to me?

Thanks :-)
Comment 6 Zeeshan Ali 2013-04-24 14:01:04 UTC
(In reply to comment #5)
> Can someone assign this bug to me?

We usually don't assign to individuals. If you are scared someone else will handle this before you can, you gotta act fast then. :)
Comment 7 Zeeshan Ali 2013-05-08 02:31:36 UTC
Atul, whats the progress on this?
Comment 8 Zeeshan Ali 2013-05-08 02:33:55 UTC
*** Bug 691495 has been marked as a duplicate of this bug. ***
Comment 9 Atul Jangra 2013-05-08 10:05:28 UTC
Hey Zeeshan, currently my practicals are going on. I'll be able to look at this around 15th May. I hope that is okay.
Comment 10 Timm Bäder 2014-03-03 13:24:44 UTC
Created attachment 270792 [details] [review]
Add a CACHEDIR variable to the configure.ac
Comment 11 Timm Bäder 2014-03-03 13:25:23 UTC
Created attachment 270793 [details] [review]
Add appropriate getters for system-wide logo and driver cache to utils.vala
Comment 12 Timm Bäder 2014-03-03 13:28:26 UTC
Created attachment 270795 [details] [review]
Use the system-wide cache directory to look for logos and drivers before downloading

This makes Downloader.download take an array of strings denoting all the directories to check for the given file before downloading(no caller ever passes more than 2 directories at this moment). Ideally, the given directories are sorted from the "most global" one to the "most local" one(i.e. from system-level to user-level). If none of the directories contains the given file, Downloader will just download it like before and save it in the most local directory(i.e. the last one in the array).
Comment 13 Zeeshan Ali 2014-03-03 17:56:34 UTC
Review of attachment 270792 [details] [review]:

If its going to be hardcoded, there is no need to involve configure* into the picture. Just define it in the Makefile.am. The vapi change also becomes redundant then.

::: src/Makefile.am
@@ +4,3 @@
 MAINTAINERCLEANFILES = *.stamp
 
+

redundant change.

@@ +9,3 @@
 	-DGNOMELOCALEDIR=\""$(datadir)/locale"\"	\
 	-DDATADIR=\""$(datadir)"\"			\
+	-DCACHEDIR=\""$(dachedir)"\" 		\

\ needs alignment with other \s.
Comment 14 Zeeshan Ali 2014-03-03 18:01:33 UTC
Review of attachment 270793 [details] [review]:

Kudos for nicely dividing your patches. Commit short log is ideally not supposed to exceed 50 characters. How about this log:

util: Add getters for system logo & driver cache

Please try to follow these guidelines for commit logs: https://wiki.gnome.org/Git/CommitMessages
Comment 15 Zeeshan Ali 2014-03-03 18:34:25 UTC
Review of attachment 270795 [details] [review]:

Looks very good as one of the very first contributions.

::: src/downloader.vala
@@ +60,3 @@
         var uri = remote_file.get_uri ();
         var download = downloads.get (uri);
+        string local_cached_path = cached_paths[cached_paths.length - 1];

* No need for the 'local_' prefix as its only making the name long.
* I'd prefer use of 'var' here as type is evident.

@@ +64,3 @@
         if (download != null)
             // Already being downloaded
+            return yield await_download (download, local_cached_path, progress);

seems we just assume in here that first path in array is where we want it. Its fine I think but we should mention this behaviour in a comment above this method.

@@ +69,1 @@
+        foreach (string cached_path in cached_paths) {

* I'd use 'var' in here too.
* To avoid conflict with the local_cached_path, that will now be renamed to cached_path, just name this very temporary variable as 'path.

@@ +84,3 @@
                 yield download_from_http (download);
             else
+                yield copy_file (remote_file, local_cached_file); // FIXME: No progress report in this case.

Since you are already getting familiar with this part of the code, I would suggest you work on fixing this FIXME after this bug.

@@ +139,2 @@
         var remote_file = File.new_for_uri (logo_url);
+        var system_cached_path = get_system_logo_cache (remote_file.get_basename ());

This part should be in a separate patch, just like I pointed out for drivers. This patch should only be about change from string to string[] of download() argument.

::: src/unattended-installer.vala
@@ +501,3 @@
+            var driver_file_name = location_checksum + "-" + file.get_basename ();
+            var cached_path = get_drivers_cache (driver_file_name);
+            var system_cached_path = get_system_drivers_cache (driver_file_name);

this part would be a seperate change than what this patch is about. You want to pass an array with just user driver cache in this patch.
Comment 16 Timm Bäder 2014-03-05 17:44:32 UTC
Review of attachment 270795 [details] [review]:

::: src/unattended-installer.vala
@@ +501,3 @@
+            var driver_file_name = location_checksum + "-" + file.get_basename ();
+            var cached_path = get_drivers_cache (driver_file_name);
+            var system_cached_path = get_system_drivers_cache (driver_file_name);

The last sentence means that I should split this patch up into 2, make the first one just about changing the parameter to string[], but to not break the build also pass a one-element array from both callers, i.e. also here -- correct?
Comment 17 Zeeshan Ali 2014-03-05 18:05:07 UTC
Review of attachment 270795 [details] [review]:

::: src/unattended-installer.vala
@@ +501,3 @@
+            var driver_file_name = location_checksum + "-" + file.get_basename ();
+            var cached_path = get_drivers_cache (driver_file_name);
+            var system_cached_path = get_system_drivers_cache (driver_file_name);

Yes since this patch is about making the argument an array rather. The next patch then uses this new *feature* to also pass the system directory.
Comment 18 Timm Bäder 2014-03-05 18:39:16 UTC
(In reply to comment #13)
> Review of attachment 270792 [details] [review]:
> 
> If its going to be hardcoded, there is no need to involve configure* into the
> picture. Just define it in the Makefile.am. The vapi change also becomes
> redundant then.

Why is the vapi change redundant? If I remove the configure.ac change, CACHEDIR will be defined just like DATADIR and that is also in the vapi.
Comment 19 Zeeshan Ali 2014-03-05 19:10:10 UTC
(In reply to comment #18)
> (In reply to comment #13)
> > Review of attachment 270792 [details] [review] [details]:
> > 
> > If its going to be hardcoded, there is no need to involve configure* into the
> > picture. Just define it in the Makefile.am. The vapi change also becomes
> > redundant then.
> 
> Why is the vapi change redundant? If I remove the configure.ac change, CACHEDIR
> will be defined just like DATADIR and that is also in the vapi.

Yeah, sorry my bad.
Comment 20 Timm Bäder 2014-03-05 19:17:31 UTC
Created attachment 271017 [details] [review]
Fixed version of the previous patch to add a CACHEDIR variable
Comment 21 Timm Bäder 2014-03-05 19:19:19 UTC
Created attachment 271018 [details] [review]
downloader: Use a string of cache dirs in download
Comment 22 Timm Bäder 2014-03-05 19:19:50 UTC
Created attachment 271019 [details] [review]
Use the system-wide cache for logos and drivers
Comment 23 Timm Bäder 2014-03-05 19:50:32 UTC
Created attachment 271024 [details] [review]
util: Add getters for system logo & driver cache
Comment 24 Zeeshan Ali 2014-03-05 20:05:06 UTC
Review of attachment 271017 [details] [review]:

Looks good but the commit log makes it sound like '/var/cache' is a temporary location.
Comment 25 Zeeshan Ali 2014-03-05 20:16:49 UTC
Review of attachment 271018 [details] [review]:

Nearly there. "Use a string of cache dirs in download" -> "Use an array of cached dirs". I'd just use "downloader: download now takes array of cached dirs" as shortlog/summary. "(which are ideally ordered from the most global to the most local directory)" part is not really true, this function doesn't exactly care about that.

::: src/downloader.vala
@@ +59,3 @@
+     *
+     * @param remote_file The remote file to download.
+     * @param cached_paths Array of cache directories to to look for the file firrst. If one of them

that is not the main purpose of this argument. The argument is just a list of directories where files are cached. Us first checking if file is already cached, is an optimization.

@@ +60,3 @@
+     * @param remote_file The remote file to download.
+     * @param cached_paths Array of cache directories to to look for the file firrst. If one of them
+     *                     contains the file already, a File object of it will be returned and nothing

I don't think this part needs to describe the return type.

@@ +61,3 @@
+     * @param cached_paths Array of cache directories to to look for the file firrst. If one of them
+     *                     contains the file already, a File object of it will be returned and nothing
+     *                     will be downloaded -- if the file is not yet locally available however, it will

Sentences are separate by '.', not '--'.

@@ +62,3 @@
+     *                     contains the file already, a File object of it will be returned and nothing
+     *                     will be downloaded -- if the file is not yet locally available however, it will
+     *                     be saved in the last element of this array.

I think it should be the first element of the array.
Comment 26 Zeeshan Ali 2014-03-05 20:28:11 UTC
Review of attachment 271019 [details] [review]:

Looks good otherwise.

::: src/downloader.vala
@@ +153,3 @@
         var cached_path = get_logo_cache (remote_file.get_basename ());
+
+        string[] cached_paths = {

just use 'var'.

::: src/unattended-installer.vala
@@ +502,3 @@
+            var cached_path = get_drivers_cache (driver_file_name);
+            var system_cached_path = get_system_drivers_cache (driver_file_name);
+            string[] cached_paths = {

use of 'var' would be appropriate'. :)

@@ +503,3 @@
+            var system_cached_path = get_system_drivers_cache (driver_file_name);
+            string[] cached_paths = {
+              system_cached_path,

is this correctly indented?
Comment 27 Zeeshan Ali 2014-03-05 20:31:38 UTC
Review of attachment 271024 [details] [review]:

looks good
Comment 28 Timm Bäder 2014-03-05 20:47:55 UTC
Review of attachment 271018 [details] [review]:

::: src/downloader.vala
@@ +62,3 @@
+     *                     contains the file already, a File object of it will be returned and nothing
+     *                     will be downloaded -- if the file is not yet locally available however, it will
+     *                     be saved in the last element of this array.

I guess this implies changing the logic of the function here and the order of the array elements in the other patch?
Comment 29 Zeeshan Ali 2014-03-05 20:54:24 UTC
Review of attachment 271018 [details] [review]:

::: src/downloader.vala
@@ +62,3 @@
+     *                     contains the file already, a File object of it will be returned and nothing
+     *                     will be downloaded -- if the file is not yet locally available however, it will
+     *                     be saved in the last element of this array.

yes, of course. :)
Comment 30 Timm Bäder 2014-03-05 21:59:03 UTC
Created attachment 271043 [details] [review]
Use the system-wide cache for logos and drivers

Fixed the wrong indentation but var can't be used in these cases.
Comment 31 Timm Bäder 2014-03-05 22:12:14 UTC
Created attachment 271045 [details] [review]
downloader: Use an array of cache dirs in download
Comment 32 Zeeshan Ali 2014-03-06 00:16:57 UTC
Review of attachment 271043 [details] [review]:

Looks good
Comment 33 Zeeshan Ali 2014-03-06 00:24:04 UTC
Review of attachment 271045 [details] [review]:

Almost there! I hope that its git-bz that somehow messed up the order and this patches comes *before* the previous patch?

::: src/downloader.vala
@@ +60,3 @@
+     * @param remote_file The remote file to download.
+     * @param cached_paths Array of cache directories. The file will be saved in the directory the
+     *                     first element points to.

saying that "The file will be saved in the first directory in the list" is sufficient but this isn't bad either.

@@ +85,3 @@
         }
 
+        var local_cached_file = GLib.File.new_for_path (cached_path);

The 'local_' prefix here is as redudant as in 'local_cached_path' that you already changed. :)

@@ +88,2 @@
         debug ("Downloading '%s'...", uri);
+        download = new Download (uri, local_cached_file, progress);

This and the following change won't be needed with the above mentioned change.
Comment 34 Timm Bäder 2014-03-06 08:59:03 UTC
Created attachment 271074 [details] [review]
downloader: Use an array of cache dirs in download

Oh, does it matter in what order I submit them to bz? I think I did that actually, sorry if that cause(d/s) problems.
Comment 35 Zeeshan Ali 2014-03-06 17:14:07 UTC
Review of attachment 271074 [details] [review]:

Just one more thing :)

::: src/downloader.vala
@@ +85,3 @@
         }
 
+        var cached_file = GLib.File.new_for_path (cached_path);

you won't need to declare it again if you bring the cached_file variable outside the foreach scope.
Comment 36 Zeeshan Ali 2014-03-06 17:20:36 UTC
(In reply to comment #34)
> Created an attachment (id=271074) [details] [review]
> downloader: Use an array of cache dirs in download
> 
> Oh, does it matter in what order I submit them to bz? 

Yeah, its not super important but its certainly makes it easy to review and then apply to repo.

>I think I did that
> actually, sorry if that cause(d/s) problems.

No worries, just wanted to say that it'd be nice to have them submitted in correct order.
Comment 37 Timm Bäder 2014-03-07 08:04:31 UTC
Created attachment 271172 [details] [review]
downloader: Use an array of cache dirs in download
Comment 38 Zeeshan Ali 2014-03-07 14:34:07 UTC
Review of attachment 271172 [details] [review]:

looks good.
Comment 39 Zeeshan Ali 2014-03-07 16:45:44 UTC
Review of attachment 271024 [details] [review]:

::: src/util.vala
@@ +55,3 @@
+        var dir = Path.build_filename (CACHEDIR, Config.PACKAGE_TARNAME);
+
+        ensure_directory (dir);

While testing the patches, the first thing I notice is that this is broken since Boxes is supposed to be run as normal user and normal user doesn't have permissions to create a directory in there. If the dir doesn't exist, this function should return null and caller should then not use system cache then.
Comment 40 Timm Bäder 2014-03-09 18:57:56 UTC
Created attachment 271369 [details] [review]
util: Add getters for system logo & driver cache

In contrary to the previous version, this one checks if directories in /var/cache exist and if they don't it returns null at the appropriate places, i.e. get_system_pkgcache, get_system_logo_cache and get_system_driver_cache.
Comment 41 Timm Bäder 2014-03-09 19:07:41 UTC
Created attachment 271370 [details] [review]
Use the system-wide cache for logos and drivers

Only uses the system cache if the appropriate directories already exist
Comment 42 Zeeshan Ali 2014-03-10 10:49:36 UTC
Review of attachment 271369 [details] [review]:

::: src/util.vala
@@ +15,3 @@
 
+    public string get_cache (string? file_name = null) {
+      return Path.build_filename (CACHEDIR, Config.PACKAGE_TARNAME, file_name);

indentation not correct here but i can fix that while merging..
Comment 43 Zeeshan Ali 2014-03-10 10:55:50 UTC
Review of attachment 271370 [details] [review]:

For future reference, please go through the checklist here to avoid my commit log nitpicks :) https://wiki.gnome.org/Git/CommitMessages

::: src/downloader.vala
@@ +153,3 @@
+        string[] cached_paths;
+
+        if (system_cached_path == null) {

Curly braces are redundant if both blocks are signle statement but i can correct that also when merging.
Comment 44 Zeeshan Ali 2014-03-10 11:18:30 UTC
All pushed! Thanks so much the work. I did modif last two patches a bit, please do a have a look at them.

Attachment 271172 [details] pushed as adcdc2e - downloader: Use an array of cache dirs in download
Attachment 271369 [details] pushed as 7408b92 - util: Add getters for system logo & driver cache