search

pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
352 stars 115 forks source link

CMake Compilation artifacts #277

Closed mrjimenez closed 3 years ago

mrjimenez commented 3 years ago

Hi,

On my cmake build, I get:

-- Found Git: /usr/bin/git (found version "2.30.2") 
-- Setting package-version to 1.9.
-- Setting package-version to 1.16.0
-- Setting ixml-version to 11.0.0
-- Setting upnp-version to 18.0.0
...

Then later I get:

[ 15%] Building C object upnp/CMakeFiles/upnp_static.dir/src/UpnpLib.c.o
In file included from /home/user/programs/pupnp/maint/github-creator/upnp/inc/UpnpLog.h:39,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/service_table.h:47,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/client_table.h:16,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/upnpapi.h:41,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/UpnpLib.h:29,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/UpnpLib.c:13:
/home/user/programs/pupnp/maint/github-creator/upnp/inc/upnpconfig.h:42: warning: "UPNP_VERSION_STRING" redefined
   42 | #define UPNP_VERSION_STRING "1.16.0"
      | 
In file included from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/config.h:36,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/UpnpLib.c:11:
/home/user/programs/pupnp/maint/github-creator/build/autoconfig.h:5: note: this is the location of the previous definition
    5 | #define UPNP_VERSION_STRING "18.0.0"
      |

I am guessing that those numbers come from the libtool numbering in configure.ac:

AC_SUBST([LT_VERSION_IXML],       [11:0:0])
AC_SUBST([LT_VERSION_UPNP],       [18:0:0])

But I also believe that it is a fair complaint from the compiler. I see two decisions to make:

  1. I have no problems in calling libupnp version 18 if that is the case, but I don't think it is the right thing to do to use libtool numbers for the library version.
  2. The current autotools build uses #define UPNP_VERSION_STRING "1.16.0" so that would make autotools and cmake builds incoherent.

Regards, Marcelo.

Vollstrecker commented 3 years ago

I am guessing that those numbers come from the libtool numbering in configure.ac:

Correct. As they produce the same library, the should use the same version. Maybe I missunderstood the ac syntax, looks like a replacement to me. And to be honest, I never looked at the internals, I just saw that the created symlink is libupnp.so.18.0.0, so it was clear to me that this is the version of the lib, while 1.16.0 is just the source-iteration. It doesn't make sense to me, I assumed that if you jump from 1.15 to 1.16 and nothing changed in ixml, that you just increase the upnp-version so the difference is explainable, but I always preferred the upnp-1.16 would produce libixml.so.1.16.0 and libupnp.so.1.16.0.

Summary (just to change it right this time). You want a 1.16 lib with an 18.0.0 symlink?

What irritates me much more is the 1.9 you get. The Setting package-version message only triggers when an uncommented AC_INIT is found, and 1.9 is not found at all in the entire code, plus this message doesn't appear in any of the config-logs of the actions, too.

mrjimenez commented 3 years ago

Hi Vollstrecker,

The libtool numbering is indeed a bit confusing. Since we are in the middle of a discussion about abandoning autotools, maybe this is a good time to fix this issue too.

Libtool numbers are like this: current:revision:age. Current - age (this is actually a subtraction arithmetic operation :laughing: ) is the number in the soname. So, if we were in libupnp:16:34:2, the library soname would be libupnp.14.so, and there would be a file named libupnp.14.so which will in fact be a link to the real file libupnp.14.2.34.so. That would allow incrementing the interface number while keeping a compatibility number. So the above scheme means that interface numbers 14, 15 and 16 can all be linked to libupnp.14.so. If you use any more recent feature, the link fails, but the program will not crash.

That can also be guaranteed by semantic versioning. So, if we go that path, we must become libupnp version 18.0.0. That looks saner in my opinion.

Libtool also handles the generation of static/dynamic objects hiding the details of position independent code in the systems that require it or do not use it. Since CMake is generating both static and dynamic versions of the library, I believe it is handling this issue.

I don't know what happens to systems that used libtool to link against libupnp, because libtool also installs *.la files that packs some clues to libtool, but I guess it would get around this as it likely does with any other non-libtool library in the system.

What do you say? Libupnp 18.0.0?

Vollstrecker commented 3 years ago

If I get this correct, this version affects the internal interface, which is checked only by libtool. pkg-config and cmake rely on other sources, and I've seen many projects that just get the info's from the headers which should report 1.x for all. As we also had the idea to get to pupnp-2.0 when the interface, object, class-thing is done, I would prefer to forget about the libtool numbering (as noone except libtool would notice) and go straight to pupnp-2.0, libupnp-2.0 and libixml-2.0.

mrjimenez commented 3 years ago

No, it is not just an internal interface. Forget about libtool, just think about the library sonames because that is what matters. Since the major has gone from libupnp.0.so to libupnp.18.so, we can't use any number less than that. Going semantic versioning means we would have to enforce these policies and numbers by hand, as opposed to libtool, where we have a little more freedom on the cost of some confusion.

In other words, what you propose would leave both libupnp and libixml in version 2.0, but the soname would have to be above or equal to 18.

I feel tempted to use 20 for the soname as the analog of 2.0, but that might bring even more confusion.

Vollstrecker commented 3 years ago

Ah that way around. Just checked at debian, they ship libupnp.so.13, so yes 2.0 would be bad. What about libupnp2.so.2.0.0. I don't know how compatible the new interface is, but with that everyone would know which one to use, and the versions could be synced. Plus it would be easier for distros to ship both, so all not ported apps stay useable.

mrjimenez commented 3 years ago

Hum, ... :laughing:

Lets see what @whyman has to say about this...

whyman commented 3 years ago

I would prefer "20, or 18" within the existing scheme, but I dont have overly strong feelings.

Vollstrecker commented 3 years ago

Just wanted to add 5 minutes ago that we would get rid of the problem with the clashing name on windows and the different names on the plattforms. Maybe this helps you feeling something more :laughing: .

whyman commented 3 years ago

Would it also involve renaming the pkg-config stuff to libupnp2 and the CMake targets too? I like the idea more if it does improve consistency, but is again more stuff for consumers to change.

Vollstrecker commented 3 years ago

I don't think so. As upnp provides ixml, too I consider this more as a package name than related to the actual libname. The work will come to the consumers anyways (if the interfaces are not compatible) as the have to ''#if'' the code for the old and new ones.

With targets you mean UPNP::Shared and that sutff? I would consider it even worse than changing the pkg-config, as we provide all information within these targets it's easier to decide in the code what to do as it's needed anyways instead of adding different targets in addition to source changes. If cmake would support some transition mechanism like if the "old target is used print a warning and use the new one" I would maybe think about it, but this isn't possible atm.

OTOH: How many consumers have already adopted the new stuff? For sure noone reports that, but if it's more than 0 (amule not counted, I'll take care of this) then we should keep that stable.

Beside the versioning: One version for all, or package=upnp and ixml stays where it is, or all the same? I would like the last one most.

mrjimenez commented 3 years ago

Hi folks, lots of information, I don't know if I get it all.

First, it is unlikely that someone has adopted CMake as of before yesterday, since CMake was unintentionally left out of the tarball. That has been fixed. Also, CMake stuff is in 1.14.4, which is pre-API-breakage. So, the way I see, CMake is going to be a thing for 1.16.x.

Now seriously, I was joking about the name stuff, since we had already settled the matter. I just though it funny because Vollstrecker new proposal was original, or maybe I didn't notice this suggestion before.

So, to move on,

  1. #define UPNP_VERSION_STRING "1.16.0" is the right thing. At least while we keep non-semantic-versioning.
  2. libupnp.18.so must be the right soname for libupnp: 
    $ readelf -a upnp/.libs/libupnp.so |  grep SONAME 
0x000000000000000e (SONAME)             Library soname: [libupnp.so.18]
  3. Also libixml.so.11 must be the right soname for libixml: 
    $ readelf -a ./ixml/.libs/libixml.so | grep SONAME                     
0x000000000000000e (SONAME)             Library soname: [libixml.so.11]
  4. The above means we need at least by now to keep the libtool insanity, i.e., current:revision:age becomes libupnp.so.(current-age).age.revision.
mrjimenez commented 3 years ago

About that warning:

[ 15%] Building C object upnp/CMakeFiles/upnp_static.dir/src/UpnpLib.c.o
In file included from /home/user/programs/pupnp/maint/github-creator/upnp/inc/UpnpLog.h:39,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/service_table.h:47,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/client_table.h:16,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/upnpapi.h:41,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/UpnpLib.h:29,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/UpnpLib.c:13:
/home/user/programs/pupnp/maint/github-creator/upnp/inc/upnpconfig.h:42: warning: "UPNP_VERSION_STRING" redefined
   42 | #define UPNP_VERSION_STRING "1.16.0"
      | 
In file included from /home/user/programs/pupnp/maint/github-creator/upnp/src/inc/config.h:36,
                 from /home/user/programs/pupnp/maint/github-creator/upnp/src/UpnpLib.c:11:
/home/user/programs/pupnp/maint/github-creator/build/autoconfig.h:5: note: this is the location of the previous definition
    5 | #define UPNP_VERSION_STRING "18.0.0"
      |

I noticed that I don't get this when I have a clean directory. By clean, I mean a directory in which I have not previously built the project with Autotools.

It has something to do with the file upnp/inc/upnpconfig.h, which is configure generated. When I delete it by hand, I get no redefinition warnings.

Maybe CMake should check for this file and delete it or find another way to make sure the other one has include priority. Or maybe we can place the autotools version somewhere else.

/ upnp/inc/upnpconfig.h. Generated from upnpconfig.h.in by configure. /

Vollstrecker commented 3 years ago

github-creator/upnp/inc/upnpconfig.h v.s. github-creator/build/autoconfig.h

I didn't check, but I guess only the second one is generated.

mrjimenez commented 3 years ago

No, they are both generated. There must be some include order priority that we are missing.

Look, this is configure.ac:

#
# There are 3 configuration files :
# 1) "./autoconfig.h" is auto-generated and used only internally during build
#    (usually named "config.h" but conflicts with the file below)
# 2) "./upnp/src/inc/config.h" is static and contains some compile-time
#    parameters. This file was previously in "./upnp/inc" but is no longer
#    installed (contains internal definitions only).
# 3) "./upnp/inc/upnpconfig.h" is auto-generated and installed with the
#    libraries : it contains information on the configuration of the
#    installed libraries.
#
AC_CONFIG_HEADERS([autoconfig.h upnp/inc/upnpconfig.h upnp/sample/common/config_sample.h])
Vollstrecker commented 3 years ago

I noticed that I don't get this when I have a clean directory. By clean, I mean a directory in which I have not previously built the project with Autotools.

That's the point. There should be no redefinition, as both are generated from the same var with the same (wrong) value. I'll take care the I change the include order while fixing that. But you now that first autoconf in the source-tree and then cmake outside should just not be done.