libostree: add versioning macros #728

GeorgesStavracas · 2017-03-10T17:41:45Z

OSTree currently provides no way to inspect the versioning
information at run time, being only available at compile
time through pkg-config.

This is a problem for e.g. Flatpak, that needs to check
whether the 'update-frequency' option is available. Checking
at compile time isn't great since it's not looking for new
symbols, but only if an optional feature is present.

This pull request, then, adds a new header that is generated
at compile time, exposing OSTree's versioning information.

cgwalters

One minor question, otherwise looks good. While it's not worth respinning for, I think general best practice nowadays is to only generate configure from configure.ac, and do everything else via sed in Makefile.am, like how we do for e.g. systemd unit files. But again, not worth respinning for.

cgwalters · 2017-03-10T17:46:57Z

src/libostree/ostree-version.h.in

+#define OSTREE_VERSION_S "@VERSION@"
+
+#define OSTREE_ENCODE_VERSION(year,release) \
+        ((year) << 16 | (release) << 8)


Why not (year) << 8 | (release) ?

Or alternatively if we want to allow ourselves more than 256 releases, (year) << 16 | (release) ?

GeorgesStavracas · 2017-03-10T17:54:25Z

Fixed the shifting. I'm not sure why the builds are failing; I'm pretty sure the headers are being exported...

cgwalters · 2017-03-10T18:05:53Z

I think it's a srcdir != builddir issue; your new header will live in the builddir. So we have to duplicate all the makefile includes to do e.g.:

diff --git a/Makefile-ostree.am b/Makefile-ostree.am
index 0b520c6..0c15cf5 100644
--- a/Makefile-ostree.am
+++ b/Makefile-ostree.am
@@ -105,6 +105,7 @@ EXTRA_DIST += src/ostree/parse-datetime.y
 CLEANFILES += src/ostree/parse-datetime.c
 
 ostree_bin_shared_cflags = $(AM_CFLAGS) -I$(srcdir)/src/libotutil -I$(srcdir)/src/libostree \
+  -I$(builddir)/src/libostree \
 	-I$(srcdir)/src/ostree -I$(srcdir)/libglnx $(OT_INTERNAL_GIO_UNIX_CFLAGS) \
 	-DPKGLIBEXECDIR=\"$(pkglibexecdir)\"
 ostree_bin_shared_ldadd = $(AM_LDFLAGS) libglnx.la libotutil.la libostree-1.la \

jlebon · 2017-03-10T21:41:57Z

apidoc/Makefile.am

@@ -34,7 +34,7 @@ DOC_MAIN_SGML_FILE=$(DOC_MODULE)-docs.xml
 # gtk-doc will search all .c & .h files beneath here for inline comments
 # documenting the functions and macros.
 # e.g. DOC_SOURCE_DIR=../../../gtk
-DOC_SOURCE_DIR=$(top_srcdir)/src/libostree
+DOC_SOURCE_DIR=$(top_builddir)/src/libostree


This still needs to be top_srcdir, no?

jlebon · 2017-03-10T21:45:29Z

src/libostree/ostree-version.h.in

+ */
+#define OSTREE_CHECK_VERSION(year,release)   \
+        (OSTREE_YEAR_VERSION > (year) || \
+         (OSTREE_YEAR_VERSION == (year) && OSTREE_RELEASE_VERSION > (release)))


Hmm, I think I would prefer the semantic for this to be >=. E.g.: https://github.com/libvirt/libvirt/blob/master/include/libvirt/libvirt-common.h.in#L87

That's a really cool addition BTW!

giuseppe · 2017-03-10T21:46:58Z

src/libostree/ostree-version.h.in

+ *
+ * ostree year version component (e.g. 2017 if %OSTREE_VERSION is 2017.2)
+ */
+#define OSTREE_YEAR_VERSION (@YEAR_VERSION@)


Should this information be exported with a function so that it is evaluated after the library is loaded? Otherwise, IMHO, it adds not much more value than pkg-config --modversion

cgwalters · 2017-03-10T22:15:00Z

@giuseppe You're 100% correct in that this PR is for a user who wants runtime dependency, and this is compile time only.

However, two things. First, I think this is nicer for compile time checking than pkg-config --modversion since one doesn't need to mess with autoconf in configure.ac (or I guess theoretically Makefile.am conditionals) in the consuming projects - you just do #ifdef FOO_CHECK_VERSION(42, 3) at each call site naturally. (Like we do with our use of libcurl today)

Second, doing compile time checking is a prerequisite IMO for having the runtime equivalent - we should steal the code from glib I'd say.

cgwalters · 2017-03-10T22:16:57Z

@GeorgesStavracas If you want I can fix it, I have a lot of autogoop experience, just let me know.

giuseppe · 2017-03-11T12:02:49Z

@cgwalters sure, I didn't mean to block the PR. At the time these new macros will be available though, we can assume all the previous features are available as well. Just wanted to point out to @GeorgesStavracas that if he needs to check a particular feature not available in older versions, he could use PKG_CHECK_MODULES to check for it.

OSTree currently provides no way to inspect the versioning information at run time, being only available at compile time through pkg-config. This is a problem for e.g. Flatpak, that needs to check whether the 'update-frequency' option is available. Checking at compile time isn't great since it's not looking for new symbols, but only if an optional feature is present. This commit, then, adds a new header that is generated at compile time, exposing OSTree's versioning information.

GeorgesStavracas · 2017-03-11T12:28:38Z

@cgwalters would be great if you could fix that, but more important than the fix itself, I'm curious to learn what's failing, and why. I'm obviously missing something related to the tests project files, for it only breaks when ci_test=yes.

cgwalters · 2017-03-11T15:13:38Z

I pushed a fixup! ⬆️

@rh-atomic-bot r+ ad2fb44

rh-atomic-bot · 2017-03-11T15:13:43Z

⌛ Testing commit ad2fb44 with merge f24da79...

OSTree currently provides no way to inspect the versioning information at run time, being only available at compile time through pkg-config. This is a problem for e.g. Flatpak, that needs to check whether the 'update-frequency' option is available. Checking at compile time isn't great since it's not looking for new symbols, but only if an optional feature is present. This commit, then, adds a new header that is generated at compile time, exposing OSTree's versioning information. Closes: #728 Approved by: cgwalters

rh-atomic-bot · 2017-03-11T15:36:44Z

💔 Test failed - status-atomicjenkins

cgwalters · 2017-03-11T15:38:55Z

Sigh, need to get libsoup patches out.

@rh-atomic-bot retry

rh-atomic-bot · 2017-03-11T15:38:59Z

⌛ Testing commit ad2fb44 with merge fda4a47...

rh-atomic-bot · 2017-03-11T15:52:07Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing fda4a47 to master...

[Previously](ostreedev#728) we added compile-time checking for versions, but there are use cases for runtime checking as well, because in a number of API calls we use `GVariant` as an API extension mechanism.

cgwalters · 2017-03-13T13:52:49Z

Followup in #735

[Previously](#728) we added compile-time checking for versions, but there are use cases for runtime checking as well, because in a number of API calls we use `GVariant` as an API extension mechanism. Closes: #735 Approved by: jlebon

cgwalters reviewed Mar 10, 2017

View reviewed changes

GeorgesStavracas force-pushed the master branch 2 times, most recently from 12130b9 to 4554150 Compare March 10, 2017 17:53

GeorgesStavracas force-pushed the master branch from 4554150 to 800b3ed Compare March 10, 2017 17:56

GeorgesStavracas force-pushed the master branch 3 times, most recently from 3fd2c37 to 51a95c4 Compare March 10, 2017 19:58

jlebon reviewed Mar 10, 2017

View reviewed changes

giuseppe reviewed Mar 10, 2017

View reviewed changes

GeorgesStavracas force-pushed the master branch from 51a95c4 to 31c61ac Compare March 11, 2017 12:22

fixup! libostree: add versioning macros

ad2fb44

rh-atomic-bot closed this in fda4a47 Mar 11, 2017

cgwalters mentioned this pull request Mar 13, 2017

core: Add runtime ostree_check_version() #735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libostree: add versioning macros #728

libostree: add versioning macros #728

GeorgesStavracas commented Mar 10, 2017

cgwalters left a comment

cgwalters Mar 10, 2017

cgwalters Mar 10, 2017 •

edited

Loading

GeorgesStavracas commented Mar 10, 2017

cgwalters commented Mar 10, 2017 •

edited

Loading

jlebon Mar 10, 2017

jlebon Mar 10, 2017

jlebon Mar 10, 2017

giuseppe Mar 10, 2017

cgwalters commented Mar 10, 2017 •

edited

Loading

cgwalters commented Mar 10, 2017

giuseppe commented Mar 11, 2017

GeorgesStavracas commented Mar 11, 2017

cgwalters commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

cgwalters commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

cgwalters commented Mar 13, 2017

libostree: add versioning macros #728

libostree: add versioning macros #728

Conversation

GeorgesStavracas commented Mar 10, 2017

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Mar 10, 2017

Choose a reason for hiding this comment

cgwalters Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

GeorgesStavracas commented Mar 10, 2017

cgwalters commented Mar 10, 2017 • edited Loading

jlebon Mar 10, 2017

Choose a reason for hiding this comment

jlebon Mar 10, 2017

Choose a reason for hiding this comment

jlebon Mar 10, 2017

Choose a reason for hiding this comment

giuseppe Mar 10, 2017

Choose a reason for hiding this comment

cgwalters commented Mar 10, 2017 • edited Loading

cgwalters commented Mar 10, 2017

giuseppe commented Mar 11, 2017

GeorgesStavracas commented Mar 11, 2017

cgwalters commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

cgwalters commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

rh-atomic-bot commented Mar 11, 2017

cgwalters commented Mar 13, 2017

cgwalters Mar 10, 2017 •

edited

Loading

cgwalters commented Mar 10, 2017 •

edited

Loading

cgwalters commented Mar 10, 2017 •

edited

Loading