library dependency information in status output (here: lz4)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

library dependency information in status output (here: lz4)

Andreas Stieger
Hello,

should we continue to add all linked dependencies (here: lz4) to the verbose version output, as the attached patch would do?

Andreas

lz4-status.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: library dependency information in status output (here: lz4)

James McCoy-3
On Tue, Dec 19, 2017 at 01:53:48PM +0100, Andreas Stieger wrote:
> should we continue to add all linked dependencies (here: lz4) to the verbose version output, as the attached patch would do?

I don't see why not.  +1 from me.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Reply | Threaded
Open this post in threaded view
|

Re: library dependency information in status output (here: lz4)

Julian Foad-5
In reply to this post by Andreas Stieger
Andreas Stieger wrote:
> Hello,
>
> should we continue to add all linked dependencies (here: lz4) to the verbose version output, as the attached patch would do?

Sounds good to me, but isn't working for me:

configure says:

checking for lz4 library via pkg-config... yes

and build says:

subversion/libsvn_subr/compress_lz4.c:30:5: warning: "SVN_INTERNAL_LZ4"
is not defined [-Wundef]
cc1: warning: unrecognized command line option ‘-Wno-unused-const-variable’
subversion/libsvn_subr/compress_lz4.c: In function
‘svn_lz4__compiled_version’:
subversion/libsvn_subr/compress_lz4.c:133:41: error:
‘LZ4_VERSION_STRING’ undeclared (first use in this function)
subversion/libsvn_subr/compress_lz4.c:133:41: note: each undeclared
identifier is reported only once for each function it appears in
subversion/libsvn_subr/compress_lz4.c: In function
‘svn_lz4__runtime_version’:
subversion/libsvn_subr/compress_lz4.c:141:10: error: implicit
declaration of function ‘LZ4_versionString’
[-Werror=implicit-function-declaration]

Subversion trunk@1818671 on Ubuntu 16.04.

Ubuntu packages 'liblz4-1' and 'liblz4-dev' version '0.0~r131-2ubuntu2'.

- Julian
Reply | Threaded
Open this post in threaded view
|

Re: library dependency information in status output (here: lz4)

Andreas Stieger
> checking for lz4 library via pkg-config... yes
>
> and build says:
>
> subversion/libsvn_subr/compress_lz4.c:30:5: warning: "SVN_INTERNAL_LZ4"

That is actually a side-effect of r1818662. Earlier versions of lz4 distributed a pkg-conf file with a Version relating to their svn revision. Your 131 is obviously not continuous with the later 1.7.5. I have not traced back which version is actually the minimum. Using LZ4_versionString() really does bump it to 1.7.5. I am not sure if this is desired? I have no particular issue with that.

Andreas
Reply | Threaded
Open this post in threaded view
|

Re: library dependency information in status output (here: lz4)

James McCoy-3
On Tue, Dec 19, 2017 at 04:46:18PM +0100, Andreas Stieger wrote:

> > checking for lz4 library via pkg-config... yes
> >
> > and build says:
> >
> > subversion/libsvn_subr/compress_lz4.c:30:5: warning: "SVN_INTERNAL_LZ4"
>
> That is actually a side-effect of r1818662. Earlier versions of lz4
> distributed a pkg-conf file with a Version relating to their svn
> revision. Your 131 is obviously not continuous with the later 1.7.5. I
> have not traced back which version is actually the minimum. Using
> LZ4_versionString() really does bump it to 1.7.5. I am not sure if
> this is desired? I have no particular issue with that.

FWIW, no version of Debian or Ubuntu has lz4 >= 1.7.5 currently.
There's an upload of 1.8.0 in Debian's experimental suite, but not one
into a suite that will end up in a release.

This is apparently due to the incompatible change in versioning, which
has caused some troubles for projects using lz4 (e.g.,
https://github.com/systemd/systemd/commit/3d4cf7de48a74726694abbaa09f9804b845ff3ba).

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Reply | Threaded
Open this post in threaded view
|

Re: library dependency information in status output (here: lz4)

James McCoy-3
In reply to this post by Andreas Stieger
On Tue, Dec 19, 2017 at 04:46:18PM +0100, Andreas Stieger wrote:
> > checking for lz4 library via pkg-config... yes
> >
> > and build says:
> >
> > subversion/libsvn_subr/compress_lz4.c:30:5: warning: "SVN_INTERNAL_LZ4"
>
> That is actually a side-effect of r1818662. Earlier versions of lz4
> distributed a pkg-conf file with a Version relating to their svn
> revision.

The headers still had the LZ4_VERSION_* defines, though.  After r131,
lz4 upstream made everything use a consisten versioning scheme and
jumped to 1.7.3.  That's also when the LZ4_VERSION_STRING define and the
corresponding function were added.

> Your 131 is obviously not continuous with the later 1.7.5. I
> have not traced back which version is actually the minimum. Using
> LZ4_versionString() really does bump it to 1.7.5. I am not sure if
> this is desired? I have no particular issue with that.

LZ4_versionNumber() has existed since r120, so we could use that and the
individual LZ4_VERSION_* defines instead.  Something like the below.

If that looks good, I'll revert r1818662, since we won't need
LZ4_versionString() anymore, and commit.

[[[
Index: subversion/include/private/svn_subr_private.h
===================================================================
--- subversion/include/private/svn_subr_private.h (revision 1818733)
+++ subversion/include/private/svn_subr_private.h (working copy)
@@ -742,7 +742,7 @@
 const char *svn_lz4__compiled_version(void);
 
 /* Return the lz4 version we run against. */
-const char *svn_lz4__runtime_version(void);
+int svn_lz4__runtime_version(void);
 
 #ifdef __cplusplus
 }
Index: subversion/libsvn_subr/compress_lz4.c
===================================================================
--- subversion/libsvn_subr/compress_lz4.c (revision 1818733)
+++ subversion/libsvn_subr/compress_lz4.c (working copy)
@@ -130,13 +130,21 @@
 const char *
 svn_lz4__compiled_version(void)
 {
-  static const char lz4_version_str[] = LZ4_VERSION_STRING;
+#define SVN_QUOTE(x) #x
+#define SVN_EXPAND(x) SVN_QUOTE(x)
+#define SVN_VERSTR(x) SVN_EXPAND(LZ4_VERSION_ ## x)
+  static const char lz4_version_str[] = SVN_VERSTR(MAJOR) "." \
+                                        SVN_VERSTR(MINOR) "." \
+                                        SVN_VERSTR(RELEASE);
+#undef SVN_QUOTE
+#undef SVN_EXPAND
+#undef SVN_VERSTR
 
   return lz4_version_str;
 }
 
-const char *
+int
 svn_lz4__runtime_version(void)
 {
-  return LZ4_versionString();
+  return LZ4_versionNumber();
 }
Index: subversion/libsvn_subr/sysinfo.c
===================================================================
--- subversion/libsvn_subr/sysinfo.c (revision 1818733)
+++ subversion/libsvn_subr/sysinfo.c (working copy)
@@ -170,8 +170,13 @@
   lib = &APR_ARRAY_PUSH(array, svn_version_ext_linked_lib_t);
   lib->name = "LZ4";
   lib->compiled_version = apr_pstrdup(pool, svn_lz4__compiled_version());
-  lib->runtime_version = apr_pstrdup(pool, svn_lz4__runtime_version());
 
+  int lz4_version = svn_lz4__runtime_version();
+  lib->runtime_version = apr_psprintf(pool, "%d.%d.%d",
+                                      lz4_version / 100 / 100,
+                                      (lz4_version / 100) % 100,
+                                      lz4_version % 100);
+
   return array;
 }
 
]]]

--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Reply | Threaded
Open this post in threaded view
|

Re: library dependency information in status output (here: lz4)

Andreas Stieger

> LZ4_versionNumber() has existed since r120, so we could use that and the
> individual LZ4_VERSION_* defines instead.  Something like the below.
[...]

From http://svn.apache.org/r1818807

-      Subversion uses LZ4 compression libary version 1.7.5 or above. Configure
+      Subversion uses LZ4 compression libary version r120 or above. Configure
[...]
-      
+
       If configure should use the version bundled with the sources, use:
         --with-lz4=internal
[...]
-      AC_MSG_ERROR([Subversion requires LZ4 >= 1.7.5, or use --with-lz4=internal])
+      AC_MSG_ERROR([Subversion requires LZ4 >= r120, or use --with-lz4=internal])
[...]
-    if $PKG_CONFIG liblz4 --atleast-version=1.7.5; then
+    if $PKG_CONFIG liblz4 --atleast-version=120 || $PKG_CONFIG liblz4 --max-version=3; then


We call LZ4_compress_default which was not added until r129.

Andreas