GNOME Bugzilla – Bug 682573
Download logos on demand
Last modified: 2016-03-31 13:55:42 UTC
SSIA, nah not really :) but check the log of attached patch which really should SIA.
Created attachment 222262 [details] [review] Download logos on demand This patch is very similar to the one I originally proposed in bug#670003. The understanding I got from Fedora legal was that distribution of the logos is not an issue but rather the use so I started to ship logos in Boxes itself. While it was really not an issue for Fedora legal, it became an issue for other distros and upstream (bug#671251). I then put this under a new package called gnome-boxes-nonfree but soon realized that most distros would not like to ship it in their main repositories as it clearly says 'nonfree' in the name. So I would like to retire that package and download these logos on demand and keep them cached. One big difference against the original patch in bug#670003 is that now we ourselves add a very tiny libosinfo database that provides logo URLs. The reason for that is that Fedora legal asked us to ensure that these logos are not used outside of Boxes' context.
Forgot to mention that I couldn't test this properly because of bug#682569.
Created attachment 222357 [details] [review] Download logos on demand V2: * I've put all the downloading and caching code in a separate new singleton class: Downloader. Later I intend to use this to also download ISOs. * Ensure same file is not downloaded more than once.
Oh and now that bug#682569 is fixed, I tested this patch.
(In reply to comment #0) > SSIA, nah not really :) but check the log of attached patch which really should > SIA. Fwiw, in this case, the subject is imo enough to get an idea of what this is about and why we want this, so SSIA could probably have worked.
Review of attachment 222357 [details] [review]: Overall looks ok, though I had quite a few comments. ::: configure.ac @@ +114,3 @@ +dnl See where we should install our custom libosinfo DB for logos +OSINFO_LOCAL_DB=`$PKG_CONFIG libosinfo-1.0 --variable=local_db_dir` I'd use the system dir here, assuming the scheme is similar to what is done for udev: https://wiki.archlinux.org/index.php/Udev#About_udev_rules "udev rules written by the administrator go in /etc/udev/rules.d/, their file name has to end with .rules. The udev rules shipped with various packages are found in /usr/lib/udev/rules.d/" ::: data/Makefile.am @@ +40,3 @@ +logosdbdir=$(OSINFO_LOCAL_DB) +logosdb_DATA= gnome-boxes-logos-db.xml This needs to be dist_logosdb_DATA or to be added to EXTRA_DIST ::: data/gnome-boxes-logos-db.xml @@ +18,3 @@ + <logo>http://design.ubuntu.com/wp-content/uploads/logo-ubuntu_cof-orange-hex.svg</logo> + </os> + Do we want to rely on these URLs never changing? Or should we mirror them somewhere? ::: src/downloader.vala @@ +4,3 @@ + private static Downloader downloader; + + private GLib.List<string> downloads; A HashTable would be more appropriate imo. @@ +25,3 @@ + + var remote_file = File.new_for_uri (uri); + var cached_path = get_user_pkgcache (remote_file.get_basename ()); I'd add a dest_dir parameter to download() so that we can put the downloaded files into a logos/ subdirectory @@ +29,3 @@ + + if (cached_file.query_exists ()) + // Good, its already cached Maybe we'll need to invalidate cached files at some point @@ +43,3 @@ + } + + public async void fetch_os_logo (Gtk.Image image, Osinfo.Os os, int size) { This looks like this could be static and get the singleton by itself instead of having every callers do it. @@ +56,3 @@ + } else { + var derived = os.get_related (Osinfo.ProductRelationship.DERIVES_FROM); + var parent = null as Osinfo.Os; This can go in the block below and does not need to be initialised to null @@ +58,3 @@ + var parent = null as Osinfo.Os; + if (derived.get_length () > 0) { + // FIXME: Does Osinfo allows deriving from multiple OSs? Also, can we have an OS that is derived from another one, that is derived from another one? Do we want to walk up the derivation chain when this happen? @@ +69,3 @@ + foreach (var i in downloads) + if (uri == i) + return true; Using a HashTable would make this function redundant @@ +79,3 @@ + SourceFunc callback = await_download.callback; + ulong downloaded_id = 0; + downloaded_id = downloaded.connect ((downloader, downloaded_uri, file) => { We may never get a signal for the uri we are interested in if an exception is raised during the copy_async for example. I'm not really sure what we want to do when this happens. Raise a "download-failed" signal so that we can do the cleanup? @@ +85,3 @@ + cached_file = file; + callback (); + disconnect (downloaded_id); A bit confused when the disconnect gets run. When the function exits? Can we put it right after the yield instead? ::: src/wizard-source.vala @@ +224,3 @@ + if (media.os != null) { + var downloader = Downloader.get_instance (); + downloader.fetch_os_logo.begin (image, media.os, 64); We don't want to wait for the end of the download here? ::: src/wizard.vala @@ +268,3 @@ + + prep_progress.fraction = 1.0; + page = WizardPage.SETUP; These duplicated 2 lines are a bit ugly, not sure how to make better though :-/
Review of attachment 222357 [details] [review]: ::: configure.ac @@ +114,3 @@ +dnl See where we should install our custom libosinfo DB for logos +OSINFO_LOCAL_DB=`$PKG_CONFIG libosinfo-1.0 --variable=local_db_dir` system dir is meant for libosinfo itself, local and user dirs are meant for apps and (advanced) users. Thats my understanding at least. We'll need Daniel to tell us which one of this interpretation is correct. Do we agree that we don't need to wait for him to be back and can fix this later if needed? ::: data/gnome-boxes-logos-db.xml @@ +18,3 @@ + <logo>http://design.ubuntu.com/wp-content/uploads/logo-ubuntu_cof-orange-hex.svg</logo> + </os> + Putting them somewhere would be very much distributing them and thats what I want to avoid with this patch. or maybe I am just being paranoid? ::: src/downloader.vala @@ +4,3 @@ + private static Downloader downloader; + + private GLib.List<string> downloads; mapping URI to corresponding local GLib.File? @@ +25,3 @@ + + var remote_file = File.new_for_uri (uri); + var cached_path = get_user_pkgcache (remote_file.get_basename ()); that was my first approach. Two reasons to make it this way: 1. IMHO its better to keep the decision of where downloaded resources are cached a private detail of this class. 2. If each download() call specifies where to download, it'll complicate the code that handles multiple downloads requests of the same resource. We can still have a separate subdir for logos, though. @@ +29,3 @@ + + if (cached_file.query_exists ()) + // Good, its already cached Maybe but I don't see any reason at the moment. There is not many logos anyways and I don't expect it to go beyond 20 (even that is unlikely). @@ +58,3 @@ + var parent = null as Osinfo.Os; + if (derived.get_length () > 0) { + // FIXME: Does Osinfo allows deriving from multiple OSs? Yes? @@ +79,3 @@ + SourceFunc callback = await_download.callback; + ulong downloaded_id = 0; + downloaded_id = downloaded.connect ((downloader, downloaded_uri, file) => { Yeah, sounds like the right thing to do. @@ +85,3 @@ + cached_file = file; + callback (); + disconnect (downloaded_id); sure ::: src/wizard-source.vala @@ +224,3 @@ + if (media.os != null) { + var downloader = Downloader.get_instance (); + downloader.fetch_os_logo.begin (image, media.os, 64); This code gets executed at app startup so no need to wait for logos to be fetched/downloaded. Worse case scenerio: logo didn't get displayed to user at wizard source page. ::: src/wizard.vala @@ +268,3 @@ + + prep_progress.fraction = 1.0; + page = WizardPage.SETUP; Yeah. I feel wizard code in general needs some serious rethinking/redesign. Willl look into that in 3.7 times..
Review of attachment 222357 [details] [review]: ::: src/downloader.vala @@ +25,3 @@ + + var remote_file = File.new_for_uri (uri); + var cached_path = get_user_pkgcache (remote_file.get_basename ()); Never mind what I said in this comment, I see your point now..
Review of attachment 222357 [details] [review]: ::: configure.ac @@ +114,3 @@ +dnl See where we should install our custom libosinfo DB for logos +OSINFO_LOCAL_DB=`$PKG_CONFIG libosinfo-1.0 --variable=local_db_dir` If system dir is meant for libosinfo itself, then it shouldn't be advertised in libosinfo .pc file. However, if we want to use this variable in this patch, and if they go in before Monday, we have to be sure that we can have a libosinfo release with these files. In the mean time, we can temporarily hardcode the path where to install the libosinfo files. ::: data/gnome-boxes-logos-db.xml @@ +18,3 @@ + <logo>http://design.ubuntu.com/wp-content/uploads/logo-ubuntu_cof-orange-hex.svg</logo> + </os> + Probably not, no idea, and not something I have given some thoughts ;) Let's see how things go this way. ::: src/downloader.vala @@ +4,3 @@ + private static Downloader downloader; + + private GLib.List<string> downloads; Mapping URI to whatever you want. The interesting feature of GHashTable VS GList is that you get constant time lookup of an uri instead of linear time lookup (and you don't need to reimplement it) @@ +25,3 @@ + + var remote_file = File.new_for_uri (uri); + var cached_path = get_user_pkgcache (remote_file.get_basename ()); I just don't want to put tons of things in the cache dir, a 'cached-downloads' directory could be fine as well. @@ +29,3 @@ + + if (cached_file.query_exists ()) + // Good, its already cached I was thinking of the remote file changing, and us needing to reload the file, bypassing the cache. @@ +58,3 @@ + var parent = null as Osinfo.Os; + if (derived.get_length () > 0) { + // FIXME: Does Osinfo allows deriving from multiple OSs? Yes what? :) Just add the iteration then?
Review of attachment 222357 [details] [review]: ::: src/downloader.vala @@ +29,3 @@ + + if (cached_file.query_exists ()) + // Good, its already cached ah ok. Any ideas? @@ +58,3 @@ + var parent = null as Osinfo.Os; + if (derived.get_length () > 0) { + // FIXME: Does Osinfo allows deriving from multiple OSs? what iteration? We keep going up the derivation lader until we find an OS that does provide the logo or there is no parent left.
Review of attachment 222357 [details] [review]: ::: src/downloader.vala @@ +58,3 @@ + var parent = null as Osinfo.Os; + if (derived.get_length () > 0) { + // FIXME: Does Osinfo allows deriving from multiple OSs? Oh bleh, recursion, missed that.
Review of attachment 222357 [details] [review]: ::: configure.ac @@ +114,3 @@ +dnl See where we should install our custom libosinfo DB for logos +OSINFO_LOCAL_DB=`$PKG_CONFIG libosinfo-1.0 --variable=local_db_dir` or we can just install in where we want/need and load it ourselves in Boxes?
Review of attachment 222357 [details] [review]: ::: configure.ac @@ +114,3 @@ +dnl See where we should install our custom libosinfo DB for logos +OSINFO_LOCAL_DB=`$PKG_CONFIG libosinfo-1.0 --variable=local_db_dir` Ah, this sounds good if libosinfo has API for you to do this
Created attachment 222798 [details] [review] Download logos on demand This patch is very similar to the one I originally proposed in bug#670003. The understanding I got from Fedora legal was that distribution of the logos is not an issue but rather the use so I started to ship logos in Boxes itself. While it was really not an issue for Fedora legal, it became an issue for other distros and upstream (bug#671251). I then put this under a new package called gnome-boxes-nonfree but soon realized that most distros would not like to ship it in their main repositories as it clearly says 'nonfree' in the name. So I would like to retire that package and download these logos on demand and keep them cached. Major differences against the original patch in bug#670003: 1. Now we ourselves maintain a very tiny libosinfo database adds logo logo URLs to some OSs. The reason for that is that Fedora legal asked us to ensure that these logos are not used outside of Boxes' context. 2. I've put all the downloading and caching code in a separate new singleton class: Downloader. Later I intend to use this to also download ISOs. 3. Ensure same file is not downloaded more than once.
Review of attachment 222798 [details] [review]: This looks good overall. My biggest outstanding concern is that this downloading is unconditional. I'm pretty sure there will be some people who will see this as a privacy issue (download of a very specific file by a specific user agent when installing a given OS for the first time). And I guess there will also be objections by others for silent download of non-free stuff. Have you checked we get a sane behaviour when the icons are missing/network is down/... ? The code looks ok, but a quick sanity test wouldn't hurt ::: data/Makefile.am @@ +39,3 @@ $(NULL) +logosdbdir= $(datadir)/gnome-boxes maybe logosdbdir= $(datadir)/gnome-boxes/osinfo ?
Review of attachment 222798 [details] [review]: ::: configure.ac @@ +53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 +OSINFO_MIN_VERSION=0.2.0 Hmm still some big uncertainties about the availability of a 0.2.0 release for next Boxes release :(
(In reply to comment #15) > Review of attachment 222798 [details] [review]: > > This looks good overall. My biggest outstanding concern is that this > downloading is unconditional. I'm pretty sure there will be some people who > will see this as a privacy issue (download of a very specific file by a > specific user agent when installing a given OS for the first time). And I guess > there will also be objections by others for silent download of non-free stuff. If thats true than I wonder why they dont object on the Fedora logo in the fedoa installation CDROM? These logos are not copyrighted but trademarked. Copying them around doesn't get you into court by misuse can. Anyway, if anyone objects a configure flag could easily be added. > Have you checked we get a sane behaviour when the icons are missing/network is > down/... ? The code looks ok, but a quick sanity test wouldn't hurt Yeah, I tested by inserting invalid URLs in the custom db. > ::: data/Makefile.am > @@ +39,3 @@ > $(NULL) > > +logosdbdir= $(datadir)/gnome-boxes > > maybe logosdbdir= $(datadir)/gnome-boxes/osinfo ? We are not going to install more than 1 db files so I don't see the point. I'm ok with giving even more specicif name to the db file. (In reply to comment #16) > Review of attachment 222798 [details] [review]: > > ::: configure.ac > @@ +53,3 @@ > SPICE_GTK_MIN_VERSION=0.12.101 > GUDEV_MIN_VERSION=165 > +OSINFO_MIN_VERSION=0.2.0 > > Hmm still some big uncertainties about the availability of a 0.2.0 release for > next Boxes release :( Ouch, I was supposed to remove that.
(In reply to comment #17) > (In reply to comment #16) > > Review of attachment 222798 [details] [review] [details]: > > > > ::: configure.ac > > @@ +53,3 @@ > > SPICE_GTK_MIN_VERSION=0.12.101 > > GUDEV_MIN_VERSION=165 > > +OSINFO_MIN_VERSION=0.2.0 > > > > Hmm still some big uncertainties about the availability of a 0.2.0 release for > > next Boxes release :( > > Ouch, I was supposed to remove that. After trying it, I realized that we need new libosinfo also for the logo API, not just for the pkg-config variables that I stopped using now. :(
(In reply to comment #18) > > After trying it, I realized that we need new libosinfo also for the logo API, > not just for the pkg-config variables that I stopped using now. :( Yes I know, I should have mentioned it ;) However, the logo stuff is just a property (+ a getter), is it possible/safe in vala to just do a g_object_get instead of using the getter? This way we'll only get a runtime warning with older libosinfo, and we don't have to rush a libosinfo release just now.
(In reply to comment #19) > (In reply to comment #18) > > > > After trying it, I realized that we need new libosinfo also for the logo API, > > not just for the pkg-config variables that I stopped using now. :( > > Yes I know, I should have mentioned it ;) However, the logo stuff is just a > property (+ a getter), is it possible/safe in vala to just do a g_object_get > instead of using the getter? This way we'll only get a runtime warning with > older libosinfo, and we don't have to rush a libosinfo release just now. There is another way, we could just use a custom parameter in the db and get that. Its pretty easy and won't exactly be a hack but lets see what danpb says about the release.
(In reply to comment #17) > > ::: data/Makefile.am > > @@ +39,3 @@ > > $(NULL) > > > > +logosdbdir= $(datadir)/gnome-boxes > > > > maybe logosdbdir= $(datadir)/gnome-boxes/osinfo ? > > We are not going to install more than 1 db files so I don't see the point. I'm > ok with giving even more specicif name to the db file. Ok, keeping the name is fine with me.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > > > > After trying it, I realized that we need new libosinfo also for the logo API, > > > not just for the pkg-config variables that I stopped using now. :( > > > > Yes I know, I should have mentioned it ;) However, the logo stuff is just a > > property (+ a getter), is it possible/safe in vala to just do a g_object_get > > instead of using the getter? This way we'll only get a runtime warning with > > older libosinfo, and we don't have to rush a libosinfo release just now. > > There is another way, we could just use a custom parameter in the db and get > that. Its pretty easy and won't exactly be a hack but lets see what danpb says > about the release. I'd favour the property as we wouldn't need to change the code/data after libosinfo release, but both approaches are fine with me. Let's see if that release is feasible.
Attachment 222798 [details] pushed as 872e9bf - Download logos on demand
Review of attachment 222357 [details] [review]: ::: src/downloader.vala @@ +4,3 @@ + private static Downloader downloader; + + private GLib.List<string> downloads; For the record, vala HashTable provides an API to make this usecase (using HashTable as a Set) more obvious: http://www.valadoc.org/#!api=glib-2.0/GLib.HashTable.add