Bug 103387 - dbus executables on Windows does not have version info
Summary: dbus executables on Windows does not have version info
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other Windows (All)
: medium normal
Assignee: Ralf Habacker
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
: 44109 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-10-21 09:59 UTC by Ralf Habacker
Modified: 2018-03-12 19:36 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add version info to installed executables for cmake build system on Windows (10.02 KB, patch)
2017-10-21 10:03 UTC, Ralf Habacker
Details | Splinter Review
Add version info to installed executables for cmake build system on Windows (10.02 KB, patch)
2017-12-05 20:48 UTC, Ralf Habacker
Details | Splinter Review
Add version info to installed executables for cmake build system on Windows (10.02 KB, patch)
2017-12-06 15:22 UTC, Ralf Habacker
Details | Splinter Review
Windows versioninfo: dbus is GPL|AFL, not LGPL|AFL (1.25 KB, patch)
2018-03-12 12:03 UTC, Simon McVittie
Details | Splinter Review
Add version info to installed executables for cmake build system on Windows (10.53 KB, patch)
2018-03-12 18:19 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2017-10-21 09:59:59 UTC
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)
Comment 1 Ralf Habacker 2017-10-21 10:03:20 UTC
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>
Comment 2 Ralf Habacker 2017-10-21 10:22:19 UTC
For a howto adding version info to automake see bug 103015 comment 95.
Comment 3 Simon McVittie 2017-10-23 10:14:24 UTC
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).
Comment 4 Ralf Habacker 2017-12-01 20:26:38 UTC
ping
Comment 5 Simon McVittie 2017-12-04 17:52:38 UTC
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.
Comment 6 Ralf Habacker 2017-12-05 20:48:56 UTC
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>
Comment 7 Ralf Habacker 2017-12-06 15:22:55 UTC
Created attachment 136006 [details] [review]
Add version info to installed executables for cmake build system on Windows

- typo fix in configure.ac
Comment 8 Ralf Habacker 2018-03-10 13:31:02 UTC
ping ?
Comment 9 Simon McVittie 2018-03-12 11:49:55 UTC
Sorry for the delay. This looks good now, please go ahead.
Comment 10 Simon McVittie 2018-03-12 12:03:17 UTC
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].
Comment 11 Ralf Habacker 2018-03-12 18:19:27 UTC
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 12 Simon McVittie 2018-03-12 18:36:03 UTC
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)
Comment 13 Ralf Habacker 2018-03-12 18:40:12 UTC
(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.
Comment 14 Ralf Habacker 2018-03-12 18:43:40 UTC
*** Bug 44109 has been marked as a duplicate of this bug. ***
Comment 15 Ralf Habacker 2018-03-12 18:51:12 UTC
(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 ?
Comment 16 Simon McVittie 2018-03-12 19:10:54 UTC
(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.
Comment 17 Ralf Habacker 2018-03-12 19:36:00 UTC
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.