Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use UNIX epochs as timestamps #20

Closed
ebassi opened this issue Jan 15, 2016 · 23 comments
Closed

Do not use UNIX epochs as timestamps #20

ebassi opened this issue Jan 15, 2016 · 23 comments

Comments

@ebassi
Copy link

ebassi commented Jan 15, 2016

UNIX epochs are a terrible way to specify a date and time, especially in a human readable and writable data format.

UNIX epochs have too high granularity for release metadata: I don't care if I released something less than a minute ago, and if something takes less than a second to release then something is clearly wrong. UNIX epochs have no actual size specified, so they could be loaded by 32 bit architectures or 64 bit ones; the AppStream API already has to use 64 bit wide unsigned integers to cope with this.

UNIX epochs do not have timezone information, as they are (generally, see below) relative to coordinate time; this means that I lose the timezone information of a release whenever I use the UNIX timestamp.

UNIX epochs are poorly defined, as there are a bunch of those - with leap seconds, without leap seconds, from UTC, from TAI, etc. You're basically encoding the behaviour of the date +%s command on your current OS whenever you use one.

Humans do not know how to write UNIX timestamps; you need an ancillary tool to write them and read them, thus introducing undeclared dependencies.

Humans have invented the ISO 8601 specification, which is what everyone should be using to represent dates and times (and intervals) inside exchange formats.

The <release/> tag in the release metadata should deprecate the timestamp attribute in favour of a new time attribute, which expresses the release time as an ISO 8601 string. The AppStream library API does not need to be modified, as it can convert to and from ISO 8601 dates transparently.

@ximion
Copy link
Owner

ximion commented Jan 15, 2016

I agree with you in every point.
The only reason UNIX epochs are used is because @hughsie simply started using them back then and they became de-facto default.
Reason for this was that his libappstream-glib doesn't have the nice Xapian cache libappstream has. That means in order to be efficient it would need to parse date/time strings and order them, which apparently hurts performance a lot (I don't care much about that in libappstream, since the Xapian read speed is what counts for it, and not the XML parsing speed - initially, the XML was never intended to serve as the primary data source, but libappsteam-glib uses it that way, and libas-glib is what powers GNOME-Software (while libas is used by KDE, Elementary and a couple of other projects)).

Admittedly, I gave in on this a bit too quickly, which is why the DEP-11 specification has a unix-timestamp field (so we could use a timestamp for something sane like YYYYDDMM etc. later).

Thing is: We can't change the AppStream XML easily now that timestamp is specified as an attribute of release, I'm afraid. Maybe adding a date attribute instead is useful.
I will talk to hughsie (Richard Hughes) about this - we now have the really dumb problem that if it makes the asglib parser slow, GNOME-Software will become slower and using it would hurt the user experience for people using that.

Anyway, let's see what Richard has to say to this again - if I change the spec on this, I want it to be implemented in libas-glib as well.

@hughsie
Copy link
Collaborator

hughsie commented Jan 15, 2016

Well, can't we have both? Have just date= (or time=, I don't care much about the name) encoded as ISO8601 in the AppData source files, and that gets converted to an epoch in the AppStream destination format. The parsing and conversion (which is slow, but not as slow as I initially feared) happens in the builder, and the runtime parser just uses the timestamp epoch for speed. That would make either the timestamp= (as an epoch) or time= in AppData files mandatory.

Of course the other way is just to accept ISO8601 dates in timestamp for AppData files, and the generator can covert them to timestamps before exporting AppStream format. That might upset the some people.

@ximion
Copy link
Owner

ximion commented Jan 15, 2016

I would probably make time/date the new default for MetaInfo files and mark the use of timestamp deprecated there (with an appropriate hint on the validator). Converting on the generator side wouldn't be a problem.
Sounds like a good compromise to me :-)

@hughsie
Copy link
Collaborator

hughsie commented Jan 16, 2016

I guess if you were to put a date on something like a wedding ring, I guess ISO8601 makes more sense indeed :)

@hughsie
Copy link
Collaborator

hughsie commented Jan 17, 2016

I've got a patch adding support for reading the ISO8601 date property on the <release> tag to libappstream-glib -- @ximion if this is okay then please let me know and I can push to master.

@ximion
Copy link
Owner

ximion commented Jan 17, 2016

I think that attribute should rather be named time instead of date since date will make people think that only dates are allowed, while time is a bit more generic (even though people will likely not use ISO8601 strings containing times down to seconds).
I will prepare the changes to the spec and libappstream tomorrow (or on Tuesday, my schedule is full of stuff...).
If you think the time attribute is okay, please go ahead with pushing changes :-)

@hughsie
Copy link
Collaborator

hughsie commented Jan 17, 2016

I don't think time is a very good name either, it gives the impression that a time is required. What about datetime?

@ximion
Copy link
Owner

ximion commented Jan 17, 2016

Well, a date is also a time, while when using date alone, allowing a time is not implied... Datetime looks like both parts are required (and it's slightly uglier than date or time).
Do you usually speak more of a release date or release time?

@hughsie
Copy link
Collaborator

hughsie commented Jan 17, 2016

I talk about release dates, the time in the day (which varies by timezone) is not important to me tbh. date then?

@hughsie
Copy link
Collaborator

hughsie commented Jan 18, 2016

Or, we have timestamp="2016-01-18" type="iso8601" and default to type=epoch...

@ximion
Copy link
Owner

ximion commented Jan 18, 2016

That would have to be timestamp_type then, since otherwise it would be a release-type (and not associated with the timestamp).
I don't like this too much, since it is pretty verbose and requires everyone writing metainfo-files to add two new attributes. It also makes ISO8601 and UNIX-epochs mutex, although that might even be a feature...

In general, I'd propose these guidelines for the specification regarding the new date/time property (called just date now, for simplicity and because I am currently slightly preferring it, since speaking of a release-date makes sense):

  • In metainfo files, the date property is preferred for stating the release-time over the timestamp property. Using timestamp is allowed, but should not be the commonly used property.
  • In distro metadata, using date or timestamp for releases is recommended. In case both properties are present, timestamp should be preferred.

@hughsie
Copy link
Collaborator

hughsie commented Jan 18, 2016

date tag or attribute? I assumed the latter, I'm not sure the date makes sense as a new tag.

@hughsie
Copy link
Collaborator

hughsie commented Jan 21, 2016

Ping? I'd like to get this in a release before I blog next. Thanks!

@ximion
Copy link
Owner

ximion commented Jan 21, 2016

Definitely no new tag... I think date as an attribute of makes the most sense.
I hope I find time to add it when I come home today (but in any case, that shouldn't (and won't) stop you).
What will you blog about? The xdg-app stuff? (because I also have an AppStream related post in the pipeline, but likely not about the same topic).

@hughsie
Copy link
Collaborator

hughsie commented Jan 21, 2016

Okay, I'll merge in later. I was going to blog again about putting release info in the AppData file, and the relation to xdg-app, but I don't want to steal your thunder if that's what you want to do :)

ximion added a commit that referenced this issue Jan 21, 2016
… time

First step towards resolving #20
The new `date` property will complement the existing `timestamp`
property on release tags.
@ximion ximion closed this as completed in 2cc0cfe Jan 21, 2016
@ximion
Copy link
Owner

ximion commented Jan 21, 2016

What I am going to write about is some general note ("write metainfo files!") and some stuff I am working on, which is a Markdown-to-metainfo file converter, since some people (me included) hate to write XML manually. Also, I wanted to blog about the "new" font support, to get some more people to write metainfo files for that. And of course, the usual does of AppStream in Debian news.
Nothing about xdg-app from my side (I'm too busy building a Limba bundle build-pipeline ^^). Congrats to the xdg-app stuff, I will need to take a look at it soon (maybe you even solved the bug preventing me from implementing updates via Limba?).
Anyway, the gist of it: Don't hold your blogpost back, doesn't hurt if people get reminded of this often ;-)

@ximion
Copy link
Owner

ximion commented Jan 21, 2016

Oh, and btw: Thanks @ebassi for the detailed issue report, it should be fixed in the next release of AppStream.
Might take a few more weeks to be fully implemented in all distro data generators though, but that shouldn't stop anyone from starting to use the tag now.

@ebassi
Copy link
Author

ebassi commented Jan 22, 2016

Thanks!

@hughsie
Copy link
Collaborator

hughsie commented Jan 22, 2016

I think you need to use more than just g_time_val_from_iso8601(); that won't accept values like "YYYY-MM-DD" -- see the commit coming up for appstream-glib...

hughsie added a commit to hughsie/appstream-glib that referenced this issue Jan 22, 2016
@ebassi
Copy link
Author

ebassi commented Jan 22, 2016

@hughsie That's actually a bug in GLib; let me quickly fix it.

@ximion
Copy link
Owner

ximion commented Jan 22, 2016

Hah, I was just going to ask if we should fix this in GLib instead of working around it in two projects.
And not noticing this bug seems to be the penalty for me not writing data-read unit-tests for the new property yesterday already...

@ebassi
Copy link
Author

ebassi commented Jan 22, 2016

@hughsie
Copy link
Collaborator

hughsie commented Jan 22, 2016

Thanks guys; when this is fixed in Glib I'll remove the workaround in appstream-glib

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants