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 690169 - Keep download cache up2date
Keep download cache up2date
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-12-13 17:07 UTC by Zeeshan Ali
Modified: 2016-03-31 13:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Revert "Download http directly via libsoup" (3.57 KB, patch)
2012-12-13 17:07 UTC, Zeeshan Ali
none Details | Review
downloader: Use mtime to keep cache up2date (2.57 KB, patch)
2012-12-13 17:07 UTC, Zeeshan Ali
none Details | Review
logos: Fetch Fedora logo from people.gnome.org (944 bytes, patch)
2012-12-13 17:08 UTC, Zeeshan Ali
committed Details | Review
downloader: Use mtime to keep cache up2date (2.62 KB, patch)
2012-12-13 18:51 UTC, Zeeshan Ali
none Details | Review
installer: Prefix driver files with location's MD5 (1.72 KB, patch)
2012-12-19 01:16 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2012-12-13 17:07:54 UTC
See patches.
Comment 1 Zeeshan Ali 2012-12-13 17:07:56 UTC
Created attachment 231491 [details] [review]
Revert "Download http directly via libsoup"

This reverts commit 16cb6ed609ae32a386844d1c32e50e34b07b36c3.

Conflicts:
	src/downloader.vala

We'll need to query and copy file attributes now (especially modification
time) in the coming patches in this series and implementing that using
libsoup isn't very simple.

Moreover using libsoup directly means we are assuming each file being
downloaded is at an HTTP server. The file (e.g in case of logos) could
as well be a local file or on an FTP server. Ensuring we work with all the
non-HTTP cases will complicate the code and we should really handle
these cases to be future safe.

Besides I dont really see any *significant* benefit of directly using
libsoup.
Comment 2 Zeeshan Ali 2012-12-13 17:07:59 UTC
Created attachment 231492 [details] [review]
downloader: Use mtime to keep cache up2date

Make use of file's modification time to ensure the cached file is in
sync with its remote sister.
Comment 3 Zeeshan Ali 2012-12-13 17:08:02 UTC
Created attachment 231493 [details] [review]
logos: Fetch Fedora logo from people.gnome.org

The wikimedia HTTP server does not seem to respond to HTTP HEAD requests
so better not use that.
Comment 4 Zeeshan Ali 2012-12-13 18:51:08 UTC
Created attachment 231502 [details] [review]
downloader: Use mtime to keep cache up2date

v2: Add forgotten 'downloaded' signal firing.
Comment 5 Alexander Larsson 2012-12-17 08:47:49 UTC
Review of attachment 231493 [details] [review]:

ack
Comment 6 Alexander Larsson 2012-12-17 09:05:14 UTC
Review of attachment 231502 [details] [review]:

Here are the issues i see with this approach:

1) It kinda works for http since it is auto-mounting, but it will not work on e.g. ftp since ftp is not auto-mounted, there instead you need to manually mount the ftp site and handle authentication etc. This will then start showing up in the users file selectors and file manager.

2) Using the generic gvfs downloader makes it impossible to do more specific things like verifying https ssl certificated against some sepecific certificat (or in fact, at all).

3) GVfs makes remote services kinda look like local files, but there is no guarantee that something like MTime is supported, so you can't rely on that. In fact, what is typically uses for something like this is for HTTP sources is the ETAG, which is generalized in GVfs via G_FILE_ATTRIBUTE_ETAG_VALUE (which for local files *does* use mtimes, but for remote sites something else).

Also, I don't think its safe to download directly to the final filename like that, if you somehow interrupt boxes while downloading it will think the file is already downloaded next time, but its partial. 
You need to either use g_file_replace() which does atomic overwrites, or copy to a temporary filename and then rename to the final name when the download is complete (and verified).
Comment 7 Christophe Fergeau 2012-12-17 10:19:20 UTC
Review of attachment 231502 [details] [review]:

I'd add to that that it doesn't work with some webservers for unknown reasons (wikimedia patch), and is buggy on others (see bug #690226)
Comment 8 Zeeshan Ali 2012-12-17 17:06:11 UTC
(In reply to comment #6)
> Review of attachment 231502 [details] [review]:
> 
> Here are the issues i see with this approach:
> 
> 1) It kinda works for http since it is auto-mounting, but it will not work on
> e.g. ftp since ftp is not auto-mounted, there instead you need to manually
> mount the ftp site and handle authentication etc. This will then start showing
> up in the users file selectors and file manager.
> 
> 2) Using the generic gvfs downloader makes it impossible to do more specific
> things like verifying https ssl certificated against some sepecific certificat
> (or in fact, at all).

These points are important. Perhaps you mentioned them before and I just forgot. :(

The thing is that currently gio/gvfs is exposed as *the* generic API for gnome apps to deal with various filesystems (local & remote). The documentation does not seem to warn the app developers about these limitations/pitfalls. Although I'm not entires sure that its the documentation that needs to be fixed since we really need such generic API for a decent app developer story. Perhaps a topic for the GNOME DX meeting in Brussels?
Comment 9 Zeeshan Ali 2012-12-17 17:33:05 UTC
Comment on attachment 231493 [details] [review]
logos: Fetch Fedora logo from people.gnome.org

Attachment 231493 [details] pushed as 370f993 - logos: Fetch Fedora logo from people.gnome.org
Comment 10 Alexander Larsson 2012-12-18 09:00:10 UTC
GIO/GVfs is really targeted at the "user files". Essentially documents and things like that. That it happens to also support http is more of a "well we can" kind of thing. The major backends for GVfs are things like local files, smb, sftp, afc, dav. I.E. Regular filesystem that the "user knows about".

The API for gio is modeled on the posix APIs, although slightly generalized for typical "document file uses" like atomic save and backups. Its possible to use it for app-internal files, but its rarely a good match. I have long wanted to add some form of app-internal mounts that are not visible in the ui and automatically unmounted as needed. That would let you do this somewhat better, but its still not a great API for a "download app", nor is it the api you want for the primary protocol support (i.e. the network engine in a browser).
Comment 11 Zeeshan Ali 2012-12-19 01:16:05 UTC
Created attachment 231857 [details] [review]
installer: Prefix driver files with location's MD5

Instead of OS ID and architecture, lets just prefix driver files with
MD5 sum of their location. This way we not only guarantee uniqueness but
also ensure that our cache is up2date. The latter assumes that location
is versioned (as it should be) so thats something we need to keep in
mind when updating drivers in libosinfo DB.
Comment 12 Alexander Larsson 2012-12-19 08:55:16 UTC
Review of attachment 231857 [details] [review]:

ack
Comment 13 Zeeshan Ali 2013-01-07 01:23:17 UTC
Comment on attachment 231857 [details] [review]
installer: Prefix driver files with location's MD5

Already committed.