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

1.14.19: pkgconfig integration contains no version #442

Open
dvzrv opened this issue Apr 25, 2024 · 11 comments
Open

1.14.19: pkgconfig integration contains no version #442

dvzrv opened this issue Apr 25, 2024 · 11 comments

Comments

@dvzrv
Copy link

dvzrv commented Apr 25, 2024

Hi! 👋

I package this project for Arch Linux.

With 1.14.19 we noticed that the pkgconfig file no longer carries version information (see https://gitlab.archlinux.org/archlinux/packaging/packages/libupnp/-/issues/1 for an upstream bug report).

We are always building from auto-generated source tarballs and therefore from our end nothing has changed since 1.14.18 (which still contains version information in the file).

cmake emits the following:

CMake Warning at CMakeLists.txt:13 (project):
  VERSION keyword not followed by a value or was followed by a value that
  expanded to nothing.
@dvzrv
Copy link
Author

dvzrv commented Apr 25, 2024

From initial investigation it appears that 470041b broke this.

https://github.com/pupnp/pupnp/blob/fb90d0ca1954bd2abf33b004e55eeb50c07cbb62/cmake/autoheader.cmake uses the files in question to derive the version via rather complex regexes. This seems rather complicated. Can an easier/ less brittle way be found to set the version?

@Vollstrecker
Copy link
Member

Unfortunately there is no easier way as libtool calculates the so-version in some "interessting" way. But from a quick look (with my mobile), the overall version should be matched correct (1.14.19 in CMakeCache), only the versions for the libs itself rely on the old format.

Can you check if it works if you remove the spaces after the comma on line 24?

@loqs
Copy link

loqs commented Apr 25, 2024

I am not sure I made the change you requested correctly:

diff --git a/cmake/autoheader.cmake b/cmake/autoheader.cmake
index df61622..883ff48 100644
--- a/cmake/autoheader.cmake
+++ b/cmake/autoheader.cmake
@@ -21,7 +21,7 @@ if (NOT PUPNP_VERSION_STRING)
 			if (line MATCHES "AC_INIT.* ([0-9]*\\.[0-9]*\\.[0-9]*).*")
 				message (STATUS "Setting package-version to ${CMAKE_MATCH_1}")
 				set (PUPNP_VERSION_STRING ${CMAKE_MATCH_1} CACHE STRING "Version of the whole package" FORCE)
-			elseif (line MATCHES "[. \t]*AC_DEFINE_UNQUOTED *\\(([^,]*), *([^,]*), *([^\\)]*)")
+			elseif (line MATCHES "[. \t]*AC_DEFINE_UNQUOTED *\\(([^,]*),*([^,]*),*([^\\)]*)")
 				set (SAVED_MATCH ${CMAKE_MATCH_1})
 
 				if ("${CMAKE_MATCH_1}" IN_LIST WRITTEN_VARS)

The result is the different in that with the change applied the build fails with:

cd /build/libupnp/src/build/upnp && /usr/bin/cc -DLIBUPNP_EXPORTS -DNDEBUG -Dupnp_shared_EXPORTS -I/build/libupnp/src/pupnp/upnp/src/threadutil -I/build/libupnp/src/pupnp/upnp/inc -I/build/libupnp/src/pupnp/upnp/src/inc -I/build/libupnp/src/build -I/build/libupnp/src/build/upnp/inc -I/build/libupnp/src/pupnp/ixml/inc -I/build/libupnp/src/pupnp/ixml/src/inc -march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions         -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security         -fstack-clash-protection -fcf-protection -g -ffile-prefix-map=/build/libupnp/src=/usr/src/debug/libupnp -flto=auto -fPIC -fvisibility=hidden -fmacro-prefix-map=/build/libupnp/src/pupnp/=. -MD -MT upnp/CMakeFiles/upnp_shared.dir/src/uuid/md5.c.o -MF CMakeFiles/upnp_shared.dir/src/uuid/md5.c.o.d -o CMakeFiles/upnp_shared.dir/src/uuid/md5.c.o -c /build/libupnp/src/pupnp/upnp/src/uuid/md5.c
/build/libupnp/src/pupnp/upnp/src/genlib/net/http/httpreadwrite.c: In function ‘get_sdk_info’:
/build/libupnp/src/build/autoconfig.h:5:29: error: too many decimal points in number
    5 | #define UPNP_VERSION_STRING 17.1.11
      |                             ^~~~~~~
/build/libupnp/src/pupnp/upnp/src/genlib/net/http/httpreadwrite.c:2311:28: note: in expansion of macro ‘UPNP_VERSION_STRING’
 2311 |                 "devices/" UPNP_VERSION_STRING "\r\n",
      |                            ^~~~~~~~~~~~~~~~~~~
/build/libupnp/src/pupnp/upnp/src/genlib/net/http/httpreadwrite.c:2311:27: error: expected ‘)’ before numeric constant
 2311 |                 "devices/" UPNP_VERSION_STRING "\r\n",
      |                           ^
      |                           )
/build/libupnp/src/pupnp/upnp/src/genlib/net/http/httpreadwrite.c:2308:17: note: to match this ‘(’
 2308 |         snprintf(info,
      |                 ^
/build/libupnp/src/pupnp/upnp/src/genlib/net/http/httpreadwrite.c:2310:19: warning: format ‘%s’ expects a matching ‘char *’ argument [-Wformat=]
 2310 |                 "%s/%s, UPnP/1.0, Portable SDK for UPnP "
      |                  ~^
      |                   |
      |                   char *
/build/libupnp/src/pupnp/upnp/src/genlib/net/http/httpreadwrite.c:2310:22: warning: format ‘%s’ expects a matching ‘char *’ argument [-Wformat=]
 2310 |                 "%s/%s, UPnP/1.0, Portable SDK for UPnP "
      |                     ~^
      |                      |
      |                      char *
In file included from /build/libupnp/src/pupnp/upnp/inc/upnp.h:47,
                 from /build/libupnp/src/pupnp/upnp/src/inc/upnputil.h:40,
                 from /build/libupnp/src/pupnp/upnp/src/inc/membuffer.h:40,
                 from /build/libupnp/src/pupnp/upnp/src/urlconfig/urlconfig.c:40:
/build/libupnp/src/build/upnp/inc/upnpconfig.h:41: warning: "UPNP_VERSION_STRING" redefined
   41 | #define UPNP_VERSION_STRING "17.1.11"
      | 
In file included from /build/libupnp/src/pupnp/upnp/src/inc/config.h:36,
                 from /build/libupnp/src/pupnp/upnp/src/urlconfig/urlconfig.c:37:
/build/libupnp/src/build/autoconfig.h:5: note: this is the location of the previous definition
    5 | #define UPNP_VERSION_STRING 17.1.11
      | 
make[2]: *** [upnp/CMakeFiles/upnp_shared.dir/build.make:328: upnp/CMakeFiles/upnp_shared.dir/src/genlib/net/http/httpreadwrite.c.o] Error 1

@loqs
Copy link

loqs commented Apr 25, 2024

Did I misunderstand you and the change you meant was:

diff --git a/cmake/autoheader.cmake b/cmake/autoheader.cmake
index df616225..e79e963f 100644
--- a/cmake/autoheader.cmake
+++ b/cmake/autoheader.cmake
diff --git a/cmake/autoheader.cmake b/cmake/autoheader.cmake
index df616225..ce655a6e 100644
--- a/cmake/autoheader.cmake
+++ b/cmake/autoheader.cmake
@@ -18,7 +18,7 @@ if (NOT PUPNP_VERSION_STRING)
 			string (REGEX REPLACE ";" "" line ${line})
 			string (REGEX REPLACE "[ \t\r\n] *" " " line ${line})
 
-			if (line MATCHES "AC_INIT.* ([0-9]*\\.[0-9]*\\.[0-9]*).*")
+			if (line MATCHES "AC_INIT.*([0-9]+\\.[0-9]+\\.[0-9]+).*")
 				message (STATUS "Setting package-version to ${CMAKE_MATCH_1}")
 				set (PUPNP_VERSION_STRING ${CMAKE_MATCH_1} CACHE STRING "Version of the whole package" FORCE)
 			elseif (line MATCHES "[. \t]*AC_DEFINE_UNQUOTED *\\(([^,]*), *([^,]*), *([^\\)]*)")

@mrjimenez
Copy link
Collaborator

Maybe we should put some comments on autoheader.cmake, it is hard to understand what those regex are doing.
You need to master regex, M4, cmake, ...
I can see that we are trying to get the version, but from where, in what order, from what part, etc, that would help a lot.

Best regards.

@Vollstrecker
Copy link
Member

@loqs I meant the first change.

It seems there should be some quotes as the version STRING is interpreted as number. At least in autoconfig.h, as the mentioned redefinition is correct. So I guess the first regex change with the right quoting in the autoconfig.h.cm (rough guess as I didn't find any call regarding this), should fix this. Or the redefinition will just have another content.

I'm back home on monday to investigate this if it's not found till then.

@mrjimenez: Sure comments would help, I hoped to never touch this again, I remember a week where I tried to replicate the behaviour of libtool based on your description. Basically it iterates over the parts of the version in LT_SUBST (?) and does the math. If I am the one to fix that, I'll find the thread from these days and put in some explanations.

@loqs
Copy link

loqs commented Apr 25, 2024

@Vollstrecker the second change I mentioned fixes it for me.

@mrjimenez
Copy link
Collaborator

I created this pull request #443, tell me what you think.

@mrjimenez
Copy link
Collaborator

Hi @Vollstrecker

@mrjimenez: Sure comments would help, I hoped to never touch this again, I remember a week where I tried to replicate the behaviour of libtool based on your description. Basically it iterates over the parts of the version in LT_SUBST (?) and does the math. If I am the one to fix that, I'll find the thread from these days and put in some explanations.

I feel your pain 😆 , regexes per si are hell, quoted regexes are a torture...

@mrjimenez
Copy link
Collaborator

However, I did notice lots of warings like this on the cmake build:

[ 75%] Building C object upnp/CMakeFiles/upnp_static.dir/src/ssdp/ssdp_server.c.o

                 from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/inc/upnputil.h:40,
                 from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/inc/membuffer.h:40,
                 from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/inc/httpparser.h:41,
                 from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/inc/ssdplib.h:45,
                 from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/ssdp/ssdp_server.c:45:
/home/mroberto/programs/pupnp/maint/github-creator/upnp/inc/upnpconfig.h:42: warning: "UPNP_VERSION_STRING" redefined
   42 | #define UPNP_VERSION_STRING "1.14.20"
      | 
In file included from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/inc/config.h:36,
                 from /home/mroberto/programs/pupnp/maint/github-creator/upnp/src/ssdp/ssdp_server.c:41:
/home/mroberto/programs/pupnp/maint/github-creator/build/autoconfig.h:5: note: this is the location of the previous definition
    5 | #define UPNP_VERSION_STRING "17.1.11"
      | 
/home/mroberto/programs/pupnp/maint/github-creator/upnp/inc/upnpconfig.h:45: warning: "UPNP_VERSION_MAJOR" redefined
   45 | #define UPNP_VERSION_MAJOR 1
      | 
/home/mroberto/programs/pupnp/maint/github-creator/build/autoconfig.h:8: note: this is the location of the previous definition
    8 | #define UPNP_VERSION_MAJOR 17
      | 
/home/mroberto/programs/pupnp/maint/github-creator/upnp/inc/upnpconfig.h:48: warning: "UPNP_VERSION_MINOR" redefined
   48 | #define UPNP_VERSION_MINOR 14
      | 
/home/mroberto/programs/pupnp/maint/github-creator/build/autoconfig.h:11: note: this is the location of the previous definition
   11 | #define UPNP_VERSION_MINOR 1
      | 
/home/mroberto/programs/pupnp/maint/github-creator/upnp/inc/upnpconfig.h:51: warning: "UPNP_VERSION_PATCH" redefined
   51 | #define UPNP_VERSION_PATCH 20
      | 
/home/mroberto/programs/pupnp/maint/github-creator/build/autoconfig.h:18: note: this is the location of the previous definition
   18 | #define UPNP_VERSION_PATCH 11
      | 

Is this normal? Is there a fix?

@mrjimenez
Copy link
Collaborator

I've also created this pull request #444 to fix those annoying warnings, but it depends on #443.

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

No branches or pull requests

4 participants