GNOME Bugzilla – Bug 698144
System cache for drivers and logos
Last modified: 2016-03-31 13:22:07 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.
Thanks for filing the bug. Can I get some code pointers to this bug. This would be helpful :-)
(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.
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 :-)
(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.
Can someone assign this bug to me? Thanks :-)
(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. :)
Atul, whats the progress on this?
*** Bug 691495 has been marked as a duplicate of this bug. ***
Hey Zeeshan, currently my practicals are going on. I'll be able to look at this around 15th May. I hope that is okay.
Created attachment 270792 [details] [review] Add a CACHEDIR variable to the configure.ac
Created attachment 270793 [details] [review] Add appropriate getters for system-wide logo and driver cache to utils.vala
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).
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.
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
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.
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?
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.
(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.
(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.
Created attachment 271017 [details] [review] Fixed version of the previous patch to add a CACHEDIR variable
Created attachment 271018 [details] [review] downloader: Use a string of cache dirs in download
Created attachment 271019 [details] [review] Use the system-wide cache for logos and drivers
Created attachment 271024 [details] [review] util: Add getters for system logo & driver cache
Review of attachment 271017 [details] [review]: Looks good but the commit log makes it sound like '/var/cache' is a temporary location.
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.
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?
Review of attachment 271024 [details] [review]: looks good
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?
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. :)
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.
Created attachment 271045 [details] [review] downloader: Use an array of cache dirs in download
Review of attachment 271043 [details] [review]: Looks good
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.
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.
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.
(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.
Created attachment 271172 [details] [review] downloader: Use an array of cache dirs in download
Review of attachment 271172 [details] [review]: looks good.
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.
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.
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
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..
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.
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