On linux os file versions are normally identified from the file name .e.g *.so.x.y . On Windows shared libraries and executables normally have version info embedded as resource info into the binary. In case of issues caused by mixed binaries from different versions version info helps to identify this. version info could be inspected from the Windows explorer (add column file version or) by using Windows API (see https://msdn.microsoft.com/en-us/library/windows/desktop/ms647003(v=vs.85).aspx). On command line there are several tools to fetch this info (see https://stackoverflow.com/questions/602802/command-line-tool-to-dump-windows-dll-version)
Created attachment 134969 [details] [review] Add version info to installed executables for cmake build system on Windows Bug: https://bugs.freedesktop.org/show_bug.cgi?id=103387 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
For a howto adding version info to automake see bug 103015 comment 95.
We've coped without this for the first 10 years of the Windows dbus port, so I don't really want to block 1.12 on it at this point. When I've re-reviewed, it can be one of the first things in 1.13, along with the feature I've been working on in Bug #100344 and its dependencies (which is why I'm so keen to get 1.12 out before I start making potentially destabilizing changes).
ping
Comment on attachment 134969 [details] [review] Add version info to installed executables for cmake build system on Windows Review of attachment 134969 [details] [review]: ----------------------------------------------------------------- ::: cmake/bus/CMakeLists.txt @@ +88,5 @@ > ${EXPAT_INCLUDE_DIR} > ) > > +if(WIN32) > + set(DBUS_INTERNAL_NAME "dbus-daemon") These three variables don't behave quite the same way as the rest of the CMake and Autotools variables, particularly the way you've used them here in CMake. So I'd be inclined to rename them to something like VERSION_INFO_INTERNAL_NAME (or VER_INTERNAL_NAME, or DBUS_VER_INTERNAL_NAME, or something), and similarly VERSION_INFO_ORIGINAL_NAME and VERSION_INFO_FILE_TYPE (or corresponding versions of whichever other naming scheme you choose), to group them together and give a hint that they all have the same unusual behaviour. @@ +129,5 @@ > + list(APPEND dbus_service_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/versioninfo-${DBUS_INTERNAL_NAME}.rc) > + add_executable(dbus-service ${dbus_service_SOURCES} ) > + target_link_libraries(dbus-service ${DBUS_INTERNAL_LIBRARIES} ${EXPAT_LIBRARIES}) > + set_target_properties(dbus-service PROPERTIES COMPILE_FLAGS ${DBUS_INTERNAL_CLIENT_DEFINITIONS}) > + install(TARGETS dbus-service ${INSTALL_TARGETS_DEFAULT_ARGS}) This looks OK, but please try not to re-indent blocks as part of a larger commit - that makes it hard to see what you actually changed, and what is just re-indentation. If you want to re-indent CMake files to use 4-space indentation steps, that's fine, but I'd prefer it as a separate commit that isn't entangled with functional changes. The CMake build system seems to be something like 50% hard tabs and 50% 4-space indentation, so it'd be good to choose one and use it consistently. ::: cmake/dbus/CMakeLists.txt @@ +241,5 @@ > ${DBUS_SHARED_HEADERS} > ) > > +if(WIN32) > + set(DBUS_INTERNAL_NAME "${CMAKE_SHARED_LIBRARY_PREFIX}dbus-1-3") I'd prefer dbus-1-${DBUS_LIBRARY_MAJOR} if you can do that. You might have to move this if(DEFINED DBUS_LIBRARY_REVISION) math(EXPR DBUS_LIBRARY_MAJOR "${DBUS_LIBRARY_CURRENT} - ${DBUS_LIBRARY_AGE}") endif() further up the file to make that work, or move this new block further down. ::: cmake/tools/CMakeLists.txt @@ +78,5 @@ > target_link_libraries(dbus-test-tool ${DBUS_LIBRARIES}) > install(TARGETS dbus-test-tool ${INSTALL_TARGETS_DEFAULT_ARGS}) > > +if(WIN32) > + # version info and uac manifest can be combined in a binary because they use different resource types Thanks, this comment is useful. ::: configure.ac @@ +155,5 @@ > + # different "internal name" and "original filename", for embedding in multiple > + # executables. In the Autotools build system, we currently only generate > + # versioninfo.rc and embed it in libdbus-1-3.dll. > + DBUS_INTERNAL_NAME=libdbus-1-3 > + AC_SUBST(DBUS_INTERNAL_NAME) This should be AC_SUBST([DBUS_INTERNAL_NAME]), with the square brackets (they act like quotes). I know configure.ac doesn't consistently get this right, but I'd like to at least get it right in new code, and eventually fix all the old code. The "3" should really be ${SOVERSION}, which is the equivalent of ${DBUS_LIBRARY_MAJOR} in the CMake (so, libdbus-1-${SOVERSION}). We set SOVERSION really early, so that should hopefully work fine. You can use AC_SUBST([DBUS_INTERNAL_NAME], [libdbus-1-${SOVERSION}]) to combine the assignment and the AC_SUBST into a single line, which is a bit more concise. The same comments apply to the other two substitutions you added here.
Created attachment 135990 [details] [review] Add version info to installed executables for cmake build system on Windows - applied requested fixes - also fixed two trailing spaces required by git commit Bug: https://bugs.freedesktop.org/show_bug.cgi?id=103387 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Created attachment 136006 [details] [review] Add version info to installed executables for cmake build system on Windows - typo fix in configure.ac
ping ?
Sorry for the delay. This looks good now, please go ahead.
Created attachment 138016 [details] [review] Windows versioninfo: dbus is GPL|AFL, not LGPL|AFL The copyright holders have not granted permission for most of dbus to be distributed under LGPL terms, so saying they have would be misleading. --- I happened to notice this while comparing Bug #44109 with this bug. Please consider applying this at the same time as Attachment #136006 [details].
Created attachment 138033 [details] [review] Add version info to installed executables for cmake build system on Windows - use GPL instead of LGPL - add 2018 as copyright year
Comment on attachment 138033 [details] [review] Add version info to installed executables for cmake build system on Windows Review of attachment 138033 [details] [review]: ----------------------------------------------------------------- Go ahead (and please apply my patch too, if you approve)
(In reply to Simon McVittie from comment #12) > Comment on attachment 138033 [details] [review] [review] > Add version info to installed executables for cmake build system on Windows > > Review of attachment 138033 [details] [review] [review]: > ----------------------------------------------------------------- > > Go ahead (and please apply my patch too, if you approve) The new patch fixes that by removing the term "Lesser" , so it is now obsolete.
*** Bug 44109 has been marked as a duplicate of this bug. ***
(In reply to Ralf Habacker from comment #13) > (In reply to Simon McVittie from comment #12) > > Comment on attachment 138033 [details] [review] [review] [review] > > Add version info to installed executables for cmake build system on Windows > > > > Review of attachment 138033 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > Go ahead (and please apply my patch too, if you approve) > > The new patch fixes that by removing the term "Lesser" , so it is now > obsolete. Do you want to have your comment added to this patch ?
(In reply to Ralf Habacker from comment #15) > > The new patch fixes that by removing the term "Lesser" , so it is now > > obsolete. Oh, even better. > Do you want to have your comment added to this patch ? No need, just apply it as-is.
patches applied to master
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.