Bug 103015 - versioninfo.o is not added to target libdbus-1 on Windows
Summary: versioninfo.o is not added to target libdbus-1 on Windows
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other Windows (All)
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/tree/win...
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-09-27 23:00 UTC by Ralf Habacker
Modified: 2017-10-26 21:39 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
log file (539.21 KB, text/plain)
2017-09-28 06:44 UTC, Ralf Habacker
Details
dbus: Make SUFFIXES more specific (717 bytes, patch)
2017-09-28 12:23 UTC, Simon McVittie
Details | Splinter Review
dbus: Clarify why we are not just adding the resource file to SOURCES (1.02 KB, patch)
2017-09-28 12:23 UTC, Simon McVittie
Details | Splinter Review
dbus: Actually link versioninfo.o into libdbus (836 bytes, patch)
2017-09-28 12:23 UTC, Simon McVittie
Details | Splinter Review
build: Remove unused variables (1006 bytes, patch)
2017-09-28 12:27 UTC, Simon McVittie
Details | Splinter Review
Windows: Simplify compiling versioninfo.rc by using libtool facilities (2.22 KB, patch)
2017-09-28 16:09 UTC, Simon McVittie
Details | Splinter Review
Windows: Use libtool support for compiling resources in tools/ too (1.06 KB, patch)
2017-09-28 16:09 UTC, Simon McVittie
Details | Splinter Review
Windows: Check for $RC, not $WINDRES (1.31 KB, patch)
2017-09-28 16:10 UTC, Simon McVittie
Details | Splinter Review
Windows: Use libtool-detected RC to compile resources in tools/ (1.22 KB, patch)
2017-09-29 12:11 UTC, Simon McVittie
Details | Splinter Review
configure.log (28.07 KB, text/x-log)
2017-10-05 22:16 UTC, Ralf Habacker
Details
make.log (437.38 KB, text/x-log)
2017-10-05 22:23 UTC, Ralf Habacker
Details
UNTESTED: cmake: Stop creating an empty afxres.h (1000 bytes, patch)
2017-10-06 12:01 UTC, Simon McVittie
Details | Splinter Review
UNTESTED: Windows: Stop manipulating line numbering in versioninfo.rc (1.38 KB, patch)
2017-10-06 12:03 UTC, Simon McVittie
Details | Splinter Review
Update versioninfo.rc.in (3.06 KB, patch)
2017-10-06 20:48 UTC, Ralf Habacker
Details | Splinter Review
- match StringFileInfo with VarFileInfo (3.16 KB, patch)
2017-10-06 21:08 UTC, Ralf Habacker
Details | Splinter Review
Update versioninfo.rc.in (3.25 KB, patch)
2017-10-17 14:46 UTC, Ralf Habacker
Details | Splinter Review
Update versioninfo.rc.in (3.38 KB, patch)
2017-10-17 14:59 UTC, Ralf Habacker
Details | Splinter Review
Use cmake build in timestamp function to generate the build time stamp (2.22 KB, patch)
2017-10-17 19:09 UTC, Ralf Habacker
Details | Splinter Review
Add version info to dbus-1 target for non msvc builds on Windows too (1.42 KB, patch)
2017-10-17 19:10 UTC, Ralf Habacker
Details | Splinter Review
- update commit message (2.83 KB, patch)
2017-10-17 19:11 UTC, Ralf Habacker
Details | Splinter Review
Update versioninfo.rc.in (3.46 KB, patch)
2017-10-18 12:37 UTC, Ralf Habacker
Details | Splinter Review
Update versioninfo.rc.in (2.87 KB, patch)
2017-10-18 13:53 UTC, Ralf Habacker
Details | Splinter Review
Fix cmake 3.5 configure error on opening a non existant file (1.08 KB, patch)
2017-10-18 13:53 UTC, Ralf Habacker
Details | Splinter Review
Add version info to installed executables for cmake build system on Windows (7.63 KB, patch)
2017-10-18 21:20 UTC, Ralf Habacker
Details | Splinter Review
Add version info to installed executables for cmake build system on Windows (9.97 KB, patch)
2017-10-19 17:58 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-09-27 23:00:57 UTC
Just detected with running 'make --trace' that in dbus subdir versioninfo.rc
is not add versioninfo.o to libdbus_1 target.
Comment 1 Ralf Habacker 2017-09-28 06:44:38 UTC
Created attachment 134523 [details]
log file
Comment 2 Simon McVittie 2017-09-28 12:07:01 UTC
I had a look at GLib's git history for comparison, and I think the problem is: we have told Automake how to build versioninfo.o; but because we are using libtool to build libdbus, it wants versioninfo.lo, a position-independent version of versioninfo.o. Shared libraries have to be PIC on some platforms, whereas objects for executables and static libraries can usually be non-PIC, and libtool enforces that distinction.
Comment 3 Simon McVittie 2017-09-28 12:07:32 UTC
Also, we're using SUFFIXES wrong. It should contain .rc, not just rc.
Comment 4 Simon McVittie 2017-09-28 12:15:47 UTC
(In reply to Simon McVittie from comment #2)
> I had a look at GLib's git history for comparison, and I think the problem
> is: we have told Automake how to build versioninfo.o; but because we are
> using libtool to build libdbus, it wants versioninfo.lo, a
> position-independent version of versioninfo.o.

That's the reason we have to do these contortions with dbus_res and dbus_res_ldflag, rather than the reason this isn't working.

The reason this isn't working is different: we define dbus_res_ldflag but we have never added it to the LDFLAGS in use. Did nobody actually test this back in 2009 when it was committed? :'-(
Comment 5 Simon McVittie 2017-09-28 12:23:03 UTC
Created attachment 134534 [details] [review]
dbus: Make SUFFIXES more specific

We want this to apply to files ending with ".rc", but not to files
ending with just "rc", like .arc or something.
Comment 6 Simon McVittie 2017-09-28 12:23:20 UTC
Created attachment 134535 [details] [review]
dbus: Clarify why we are not just adding the resource  file to SOURCES
Comment 7 Simon McVittie 2017-09-28 12:23:38 UTC
Created attachment 134536 [details] [review]
dbus: Actually link versioninfo.o into libdbus

It appears this has been wrong ever since the versioninfo machinery
was first added in 2009, and nobody noticed until now.
Comment 8 Simon McVittie 2017-09-28 12:27:49 UTC
Created attachment 134537 [details] [review]
build: Remove unused variables

libdbus isn't localized, so we have no use for libintl. We always
link libdbus-1 with -no-undefined, so we have no use for
putting that flag in no_undefined on Windows only. export_symbols
seems to be left over from before Bug #83115 was fixed.
Comment 9 Simon McVittie 2017-09-28 12:37:22 UTC
In the version I'd merge, I changed the "dbus:" prefixes to "Windows:" to be a bit more specific. I'm not going to bother reattaching the patches for that.

https://github.com/smcv/dbus/tree/windows-versioninfo-103015
Comment 10 Philip Withnall 2017-09-28 12:44:24 UTC
Comment on attachment 134534 [details] [review]
dbus: Make SUFFIXES more specific

Review of attachment 134534 [details] [review]:
-----------------------------------------------------------------

++
Comment 11 Philip Withnall 2017-09-28 12:45:06 UTC
Comment on attachment 134535 [details] [review]
dbus: Clarify why we are not just adding the resource  file to SOURCES

Review of attachment 134535 [details] [review]:
-----------------------------------------------------------------

++
Comment 12 Philip Withnall 2017-09-28 12:45:26 UTC
Comment on attachment 134536 [details] [review]
dbus: Actually link versioninfo.o into libdbus

Review of attachment 134536 [details] [review]:
-----------------------------------------------------------------

++
Comment 13 Philip Withnall 2017-09-28 12:46:37 UTC
Comment on attachment 134537 [details] [review]
build: Remove unused variables

Review of attachment 134537 [details] [review]:
-----------------------------------------------------------------

++
Comment 14 Ralf Habacker 2017-09-28 12:49:59 UTC
(In reply to Simon McVittie from comment #4)
> (In reply to Simon McVittie from comment #2)
> > I had a look at GLib's git history for comparison, and I think the problem
> > is: we have told Automake how to build versioninfo.o; but because we are
> > using libtool to build libdbus, it wants versioninfo.lo, a
> > position-independent version of versioninfo.o.

I just found this https://stackoverflow.com/questions/17493786/how-to-add-copyright-notice-and-vesion-information-to-dll-using-autotools

would that work instead ?
Comment 15 Ralf Habacker 2017-09-28 13:26:06 UTC
(In reply to Ralf Habacker from comment #14)
> (In reply to Simon McVittie from comment #4)
> > (In reply to Simon McVittie from comment #2)
> > > I had a look at GLib's git history for comparison, and I think the problem
> > > is: we have told Automake how to build versioninfo.o; but because we are
> > > using libtool to build libdbus, it wants versioninfo.lo, a
> > > position-independent version of versioninfo.o.
> 
> I just found this
> https://stackoverflow.com/questions/17493786/how-to-add-copyright-notice-and-
> vesion-information-to-dll-using-autotools
> 
> would that work instead ?

Here is another thread which mentions the .rc.lo rule http://gnu-automake.7480.n7.nabble.com/correct-windres-use-td4889.html
Comment 16 Simon McVittie 2017-09-28 14:40:00 UTC
(In reply to Ralf Habacker from comment #14)
> I just found this
> https://stackoverflow.com/questions/17493786/how-to-add-copyright-notice-and-
> vesion-information-to-dll-using-autotools
> 
> would that work instead ?

Quite possibly. Can we merge what I have here, which keeps the current infrastructure but makes it actually work, and then try the .rc.lo thing as subsequent cleanup?
Comment 17 Ralf Habacker 2017-09-28 14:56:02 UTC
Comment on attachment 134534 [details] [review]
dbus: Make SUFFIXES more specific

applied to master
Comment 18 Ralf Habacker 2017-09-28 14:56:14 UTC
Comment on attachment 134535 [details] [review]
dbus: Clarify why we are not just adding the resource  file to SOURCES

applied to master
Comment 19 Ralf Habacker 2017-09-28 14:56:28 UTC
Comment on attachment 134536 [details] [review]
dbus: Actually link versioninfo.o into libdbus

applied to master
Comment 20 Ralf Habacker 2017-09-28 14:56:39 UTC
Comment on attachment 134537 [details] [review]
build: Remove unused variables

applied to master
Comment 21 Simon McVittie 2017-09-28 16:09:28 UTC
Created attachment 134544 [details] [review]
Windows: Simplify compiling versioninfo.rc by using  libtool facilities

libtool has built-in support for Windows resources, and we even
enable it in configure.ac. What it doesn't have is a built-in rule
for generating Libtool objects using that built-in support, but
we can add one.

We have to generate Libtool pseudo-objects (.lo) rather than native
object files (.o) so that we get both a PIC object for the shared
library and a non-PIC object for the static library.

This mimics the libtool invocations used for compiling C and C++.
Note that $(RC) is typically i686-w64-mingw32-windres, the same as
our project-specific variable $(WINDRES) which was previously used here.
Comment 22 Simon McVittie 2017-09-28 16:09:45 UTC
Created attachment 134545 [details] [review]
Windows: Use libtool support for compiling resources in  tools/ too

We might as well be consistent with dbus/ here.

Note that unlike the version in dbus/ we have to build a plain object
file here, not a Libtool object (.o, not .lo) because it's going to
go into an executable, not a shared library. That's the only difference.
Comment 23 Simon McVittie 2017-09-28 16:10:03 UTC
Created attachment 134546 [details] [review]
Windows: Check for $RC, not $WINDRES

That's what is checked for by LT_LANG([Windows Resource]) further
up, and is what we now use during the build. Its value is typically
i686-w64-mingw32-windres.
Comment 24 Ralf Habacker 2017-09-28 16:22:51 UTC
(In reply to Simon McVittie from comment #22)
> Created attachment 134545 [details] [review] [review]
> Windows: Use libtool support for compiling resources in  tools/ too
> 
> We might as well be consistent with dbus/ here.
> 
> Note that unlike the version in dbus/ we have to build a plain object
> file here, not a Libtool object (.o, not .lo) because it's going to
> go into an executable, not a shared library. That's the only difference.

I tried that and got 

/bin/sh ../libtool  --tag=RC   --mode=compile i686-w64-mingw32-windres disable-uac.rc -o disable-uac.o
libtool: compile:  i686-w64-mingw32-windres disable-uac.rc  -o .libs/disable-uac.o
/bin/sh ../libtool  --tag=CC   --mode=link i686-w64-mingw32-gcc   -fno-strict-aliasing -fno-common -Wall -Wextra -Wundef -Wnested-externs -Wwrite-strings -Wpointer-arith -Wmissing-declarations -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wno-unused-parameter -Wno-missing-field-initializers -Wdeclaration-after-statement -Wformat=2 -Wold-style-definition -Wcast-align -Wformat-nonliteral -Wformat-security -Wsign-compare -Wstrict-aliasing -Wshadow -Winline -Wpacked -Wmissing-format-attribute -Wmissing-noreturn -Winit-self -Wmissing-include-dirs -Wunused-but-set-variable -Warray-bounds -Wimplicit-function-declaration -Wreturn-type -Wswitch-enum -Wswitch-default -Wchar-subscripts -Wfloat-equal -Wpointer-sign -Wno-error=unused-parameter -Wno-error=missing-field-initializers  -export-dynamic -Wl,--no-as-needed  -o dbus-update-activation-environment.exe dbus-update-activation-environment.o tool-common.o disable-uac.o ../dbus/libdbus-1.la 
libtool: link: i686-w64-mingw32-gcc -fno-strict-aliasing -fno-common -Wall -Wextra -Wundef -Wnested-externs -Wwrite-strings -Wpointer-arith -Wmissing-declarations -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wno-unused-parameter -Wno-missing-field-initializers -Wdeclaration-after-statement -Wformat=2 -Wold-style-definition -Wcast-align -Wformat-nonliteral -Wformat-security -Wsign-compare -Wstrict-aliasing -Wshadow -Winline -Wpacked -Wmissing-format-attribute -Wmissing-noreturn -Winit-self -Wmissing-include-dirs -Wunused-but-set-variable -Warray-bounds -Wimplicit-function-declaration -Wreturn-type -Wswitch-enum -Wswitch-default -Wchar-subscripts -Wfloat-equal -Wpointer-sign -Wno-error=unused-parameter -Wno-error=missing-field-initializers -Wl,--no-as-needed -o .libs/dbus-update-activation-environment.exe dbus-update-activation-environment.o tool-common.o disable-uac.o -Wl,--export-all-symbols  ../dbus/.libs/libdbus-1.dll.a -lws2_32 -liphlpapi -ldbghelp /usr/lib64/gcc/i686-w64-mingw32/7.2.0/libstdc++.dll.a -L/usr/i686-w64-mingw32/sys-root/mingw/lib -L/usr/lib64/gcc/i686-w64-mingw32/7.2.0
i686-w64-mingw32-gcc: error: disable-uac.o: No such file or directory

disable-uac.o lives in .libs
Comment 25 Ralf Habacker 2017-09-28 16:32:48 UTC
(In reply to Ralf Habacker from comment #24)
automake-1.13.4-8.9.noarch
autoconf-2.69-12.49.noarch
libtool-2.4.2-18.3.1.x86_64
m4-1.4.16-4.5.x86_64
Comment 26 Philip Withnall 2017-09-28 16:39:46 UTC
Comment on attachment 134544 [details] [review]
Windows: Simplify compiling versioninfo.rc by using  libtool facilities

Review of attachment 134544 [details] [review]:
-----------------------------------------------------------------

This looks reasonable to me from an autotools POV, but I have no idea about the libtool/Windows side of things. ++
Comment 27 Philip Withnall 2017-09-28 16:40:05 UTC
Comment on attachment 134545 [details] [review]
Windows: Use libtool support for compiling resources in  tools/ too

Review of attachment 134545 [details] [review]:
-----------------------------------------------------------------

This looks reasonable to me from an autotools POV, but I have no idea about the libtool/Windows side of things. ++
Comment 28 Philip Withnall 2017-09-28 16:40:37 UTC
Comment on attachment 134546 [details] [review]
Windows: Check for $RC, not $WINDRES

Review of attachment 134546 [details] [review]:
-----------------------------------------------------------------

++
Comment 29 Simon McVittie 2017-09-29 12:11:40 UTC
Created attachment 134565 [details] [review]
Windows: Use libtool-detected RC to compile resources in  tools/

We have two variables that both expand to i686-w64-mingw32-windres,
namely WINDRES and RC, and we might as well use the same one as
in dbus/ here. However, it seems we can't wrap windres in libtool
when producing an executable: if we use .rc.lo, my Automake 1.15.1
doesn't realise that it needs to include disable-uac.lo in the
list of objects, whereas if we use .rc.o, Ralf's libtool 2.4.2 and
Automake 1.13.4 disagree on where the output should go
(.libs/disable-uac.o vs. disable-uac.o) and the link fails.

---

A more minimal version of Attachment #134545 [details] that hopefully works for both of us. Attachment #134546 [details] cannot be applied before this one.

Is there any particular reason why your build system is using a libtool version that was superseded 3 years ago and an Automake that was superseded 4 years ago?
Comment 30 Simon McVittie 2017-09-29 12:21:10 UTC
(In reply to Simon McVittie from comment #29)
> Is there any particular reason why your build system is using a libtool
> version that was superseded 3 years ago and an Automake that was superseded
> 4 years ago?

Normally I'd say "you should update, otherwise WONTFIX", but I can't immediately find anything in libtool's revision history that looks like it would fix this, so I'm not sure why mine works and yours doesn't...

Attachment #134544 [details] is still a win (assuming it works for you - I don't think you'd have got as far as building tools/ if it didn't), and Attachment #134565 [details] is still a bit of a simplification (because it makes Attachment #134546 [details] possible), so I think this series is worth applying.
Comment 31 Philip Withnall 2017-09-29 12:27:53 UTC
Comment on attachment 134565 [details] [review]
Windows: Use libtool-detected RC to compile resources in  tools/

Review of attachment 134565 [details] [review]:
-----------------------------------------------------------------

This also looks good, given the previous change to check for $RC rather than $WINDRES.

I do feel like I’m acking things without a clear understanding of what’s going on, though, so please take this with a pinch of salt.
Comment 32 Ralf Habacker 2017-09-29 21:07:26 UTC
(In reply to Simon McVittie from comment #29)
> Is there any particular reason why your build system is using a libtool
> version that was superseded 3 years ago and an Automake that was superseded
> 4 years ago?

2.4.2 is the offical libtool release on recent openSUSE distribution (see https://software.opensuse.org/package/libtool)
Comment 33 Ralf Habacker 2017-09-29 21:55:21 UTC
(In reply to Ralf Habacker from comment #32)
> (In reply to Simon McVittie from comment #29)
> > Is there any particular reason why your build system is using a libtool
> > version that was superseded 3 years ago and an Automake that was superseded
> > 4 years ago?
> 
> 2.4.2 is the offical libtool release on recent openSUSE distribution (see
> https://software.opensuse.org/package/libtool)

I tried both versions, the result is the same. :-(

In the error case running make V=1 returns 
/bin/sh ../libtool  --tag=RC   --mode=compile i686-w64-mingw32-windres disable-uac.rc -o disable-uac.o
libtool: compile:  i686-w64-mingw32-windres disable-uac.rc  -o .libs/disable-uac.o

wich means using libtool here is one of the issue, so I reverted attachment 134544 [details] [review]. 

Additional using $(RC) instead of $(WINDRES) (caused by attachment 134546 [details] [review]) works without any remaining issue.
Comment 34 Simon McVittie 2017-10-02 08:45:08 UTC
(In reply to Ralf Habacker from comment #33)
> In the error case running make V=1 returns 
> /bin/sh ../libtool  --tag=RC   --mode=compile i686-w64-mingw32-windres
> disable-uac.rc -o disable-uac.o
> libtool: compile:  i686-w64-mingw32-windres disable-uac.rc  -o
> .libs/disable-uac.o
> 
> wich means using libtool here is one of the issue, so I reverted attachment
> 134544.

Please don't revert Attachment #134544 [details] (a change to dbus/) just because the part in tools/ doesn't work. Please test/review the part in dbus/, and the part in tools/, individually and on their own merits.

The .rc file in dbus/ is getting linked into a shared library using libtool, via a .lo pseudo-object, so it's fine that we're using libtool to compile the pseudo-object, because we are also using libtool to link the shared library, and libtool knows that listing versioninfo.lo on the link line actually means to use the corresponding .libs/*.o.

The .rc file in tools/ is getting linked directly into an executable, so the situation there is a bit different, and I'm less surprised that going via libtool doesn't work as intended there. I would hope that removing Attachment #134545 [details] and using Attachment #134565 [details] instead would make everything work for you.

To be entirely clear about this, what I'm currently proposing is git master + Attachment #134544 [details] + Attachment #134565 [details] + Attachment #134546 [details] (in that order). If that doesn't work, please could you show me a log for that from your build infrastructure?
Comment 35 Simon McVittie 2017-10-02 09:06:02 UTC
What I tried: https://github.com/smcv/dbus/tree/resource-compiler-103015
Build log: https://travis-ci.org/smcv/dbus/jobs/282157277

This is on travis-ci's Ubuntu 14.04 system, which doesn't actually tell us which versions it's using in the log, but according to Launchpad the answer should be Autoconf 2.69-6, Automake 1:1.14.1-2ubuntu1 and libtool 2.4.2-1.7ubuntu1.

In dbus/
========

libtool builds versioninfo.o twice (once nominally PIC and once non-PIC, although there is no actual difference):

5585 /bin/bash ../libtool  --tag=RC   --mode=compile i686-w64-mingw32-windres versioninfo.rc -o versioninfo.lo
5586 libtool: compile:  i686-w64-mingw32-windres versioninfo.rc  -o .libs/versioninfo.o
5587 libtool: compile:  i686-w64-mingw32-windres versioninfo.rc -o versioninfo.o >/dev/null 2>&1

and .libs/versioninfo.o gets linked into the shared library libdbus-1-3.dll, while versioninfo.o gets linked into the static library libdbus-1.a:

5783 /bin/bash ../libtool  --tag=CC   --mode=link i686-w64-mingw32-gcc    -fno-strict-aliasing -fno-common -Wall -Wextra -Wundef -Wnested-externs -Wwrite-strings -Wpointer-arith -Wmissing-declarations -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wno-unused-parameter -Wno-missing-field-initializers -Wdeclaration-after-statement -Wformat=2 -Wold-style-definition -Wcast-align -Wformat-nonliteral -Wformat-security -Wsign-compare -Wstrict-aliasing -Wshadow -Winline -Wpacked -Wmissing-format-attribute -Wmissing-noreturn -Winit-self -Wmissing-include-dirs -Wunused-but-set-variable -Warray-bounds -Wimplicit-function-declaration -Wreturn-type -Wswitch-enum -Wswitch-default -Wchar-subscripts -Wfloat-equal -Wpointer-sign -Wno-unused-label -Wno-error=unused-parameter -Wno-error=missing-field-initializers -Wno-error=unused-label -static-libgcc  -version-info 22:0:19 -Wl,--version-script=Version -no-undefined  -Wl,--no-as-needed  -o libdbus-1.la -rpath /usr/local/lib libdbus_1_la-dbus-address.lo libdbus_1_la-dbus-auth.lo libdbus_1_la-dbus-bus.lo libdbus_1_la-dbus-connection.lo libdbus_1_la-dbus-credentials.lo libdbus_1_la-dbus-errors.lo libdbus_1_la-dbus-keyring.lo libdbus_1_la-dbus-marshal-header.lo libdbus_1_la-dbus-marshal-byteswap.lo libdbus_1_la-dbus-marshal-recursive.lo libdbus_1_la-dbus-marshal-validate.lo libdbus_1_la-dbus-message.lo libdbus_1_la-dbus-misc.lo libdbus_1_la-dbus-nonce.lo libdbus_1_la-dbus-object-tree.lo libdbus_1_la-dbus-pending-call.lo libdbus_1_la-dbus-resources.lo libdbus_1_la-dbus-server.lo libdbus_1_la-dbus-server-debug-pipe.lo libdbus_1_la-dbus-server-socket.lo libdbus_1_la-dbus-server-win.lo versioninfo.lo libdbus_1_la-dbus-sha.lo libdbus_1_la-dbus-signature.lo libdbus_1_la-dbus-syntax.lo libdbus_1_la-dbus-timeout.lo libdbus_1_la-dbus-threads.lo libdbus_1_la-dbus-transport.lo libdbus_1_la-dbus-transport-socket.lo libdbus_1_la-dbus-watch.lo libdbus_1_la-dbus-dataslot.lo libdbus_1_la-dbus-file.lo libdbus_1_la-dbus-hash.lo libdbus_1_la-dbus-internals.lo libdbus_1_la-dbus-list.lo libdbus_1_la-dbus-marshal-basic.lo libdbus_1_la-dbus-memory.lo libdbus_1_la-dbus-mempool.lo libdbus_1_la-dbus-pipe.lo libdbus_1_la-dbus-string.lo  libdbus_1_la-dbus-file-win.lo libdbus_1_la-dbus-pipe-win.lo libdbus_1_la-dbus-sysdeps-win.lo libdbus_1_la-dbus-sysdeps-thread-win.lo libdbus_1_la-dbus-transport-win.lo libdbus_1_la-dbus-sysdeps.lo -lws2_32 -liphlpapi -ldbghelp  libdbus-init-win.la 
5784 libtool: link: i686-w64-mingw32-gcc -shared  .libs/libdbus_1_la-dbus-address.o .libs/libdbus_1_la-dbus-auth.o .libs/libdbus_1_la-dbus-bus.o .libs/libdbus_1_la-dbus-connection.o .libs/libdbus_1_la-dbus-credentials.o .libs/libdbus_1_la-dbus-errors.o .libs/libdbus_1_la-dbus-keyring.o .libs/libdbus_1_la-dbus-marshal-header.o .libs/libdbus_1_la-dbus-marshal-byteswap.o .libs/libdbus_1_la-dbus-marshal-recursive.o .libs/libdbus_1_la-dbus-marshal-validate.o .libs/libdbus_1_la-dbus-message.o .libs/libdbus_1_la-dbus-misc.o .libs/libdbus_1_la-dbus-nonce.o .libs/libdbus_1_la-dbus-object-tree.o .libs/libdbus_1_la-dbus-pending-call.o .libs/libdbus_1_la-dbus-resources.o .libs/libdbus_1_la-dbus-server.o .libs/libdbus_1_la-dbus-server-debug-pipe.o .libs/libdbus_1_la-dbus-server-socket.o .libs/libdbus_1_la-dbus-server-win.o .libs/versioninfo.o .libs/libdbus_1_la-dbus-sha.o .libs/libdbus_1_la-dbus-signature.o .libs/libdbus_1_la-dbus-syntax.o .libs/libdbus_1_la-dbus-timeout.o .libs/libdbus_1_la-dbus-threads.o .libs/libdbus_1_la-dbus-transport.o .libs/libdbus_1_la-dbus-transport-socket.o .libs/libdbus_1_la-dbus-watch.o .libs/libdbus_1_la-dbus-dataslot.o .libs/libdbus_1_la-dbus-file.o .libs/libdbus_1_la-dbus-hash.o .libs/libdbus_1_la-dbus-internals.o .libs/libdbus_1_la-dbus-list.o .libs/libdbus_1_la-dbus-marshal-basic.o .libs/libdbus_1_la-dbus-memory.o .libs/libdbus_1_la-dbus-mempool.o .libs/libdbus_1_la-dbus-pipe.o .libs/libdbus_1_la-dbus-string.o .libs/libdbus_1_la-dbus-file-win.o .libs/libdbus_1_la-dbus-pipe-win.o .libs/libdbus_1_la-dbus-sysdeps-win.o .libs/libdbus_1_la-dbus-sysdeps-thread-win.o .libs/libdbus_1_la-dbus-transport-win.o .libs/libdbus_1_la-dbus-sysdeps.o  -Wl,--whole-archive ./.libs/libdbus-init-win.a -Wl,--no-whole-archive  -lws2_32 -liphlpapi -ldbghelp  -Wl,--version-script=Version -Wl,--no-as-needed   -o .libs/libdbus-1-3.dll -Wl,--enable-auto-image-base -Xlinker --out-implib -Xlinker .libs/libdbus-1.dll.a
...
5787 libtool: link: i686-w64-mingw32-ar cru .libs/libdbus-1.a  libdbus_1_la-dbus-address.o libdbus_1_la-dbus-auth.o libdbus_1_la-dbus-bus.o libdbus_1_la-dbus-connection.o libdbus_1_la-dbus-credentials.o libdbus_1_la-dbus-errors.o libdbus_1_la-dbus-keyring.o libdbus_1_la-dbus-marshal-header.o libdbus_1_la-dbus-marshal-byteswap.o libdbus_1_la-dbus-marshal-recursive.o libdbus_1_la-dbus-marshal-validate.o libdbus_1_la-dbus-message.o libdbus_1_la-dbus-misc.o libdbus_1_la-dbus-nonce.o libdbus_1_la-dbus-object-tree.o libdbus_1_la-dbus-pending-call.o libdbus_1_la-dbus-resources.o libdbus_1_la-dbus-server.o libdbus_1_la-dbus-server-debug-pipe.o libdbus_1_la-dbus-server-socket.o libdbus_1_la-dbus-server-win.o versioninfo.o libdbus_1_la-dbus-sha.o libdbus_1_la-dbus-signature.o libdbus_1_la-dbus-syntax.o libdbus_1_la-dbus-timeout.o libdbus_1_la-dbus-threads.o libdbus_1_la-dbus-transport.o libdbus_1_la-dbus-transport-socket.o libdbus_1_la-dbus-watch.o libdbus_1_la-dbus-dataslot.o libdbus_1_la-dbus-file.o libdbus_1_la-dbus-hash.o libdbus_1_la-dbus-internals.o libdbus_1_la-dbus-list.o libdbus_1_la-dbus-marshal-basic.o libdbus_1_la-dbus-memory.o libdbus_1_la-dbus-mempool.o libdbus_1_la-dbus-pipe.o libdbus_1_la-dbus-string.o libdbus_1_la-dbus-file-win.o libdbus_1_la-dbus-pipe-win.o libdbus_1_la-dbus-sysdeps-win.o libdbus_1_la-dbus-sysdeps-thread-win.o libdbus_1_la-dbus-transport-win.o libdbus_1_la-dbus-sysdeps.o  .libs/libdbus-1.lax/libdbus-init-win.a/dbus-init-win.o 

In tools/
=========

We compile disable-uac.rc to disable-uac.o, this time without libtool, then link it into dbus-update-activation-environment.exe:

5933 i686-w64-mingw32-windres disable-uac.rc -o disable-uac.o
5934 /bin/bash ../libtool  --tag=CC   --mode=link i686-w64-mingw32-gcc   -fno-strict-aliasing -fno-common -Wall -Wextra -Wundef -Wnested-externs -Wwrite-strings -Wpointer-arith -Wmissing-declarations -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wno-unused-parameter -Wno-missing-field-initializers -Wdeclaration-after-statement -Wformat=2 -Wold-style-definition -Wcast-align -Wformat-nonliteral -Wformat-security -Wsign-compare -Wstrict-aliasing -Wshadow -Winline -Wpacked -Wmissing-format-attribute -Wmissing-noreturn -Winit-self -Wmissing-include-dirs -Wunused-but-set-variable -Warray-bounds -Wimplicit-function-declaration -Wreturn-type -Wswitch-enum -Wswitch-default -Wchar-subscripts -Wfloat-equal -Wpointer-sign -Wno-unused-label -Wno-error=unused-parameter -Wno-error=missing-field-initializers -Wno-error=unused-label -static-libgcc  -Wl,--no-as-needed  -o dbus-update-activation-environment.exe dbus-update-activation-environment.o tool-common.o disable-uac.o ../dbus/libdbus-1.la 

So this all seems OK.
Comment 36 Simon McVittie 2017-10-05 17:46:37 UTC
Sorry, I'm not going to make any further progress on this without a complete build log.
Comment 37 Ralf Habacker 2017-10-05 22:16:29 UTC
Created attachment 134695 [details]
configure.log
Comment 38 Ralf Habacker 2017-10-05 22:23:57 UTC
Created attachment 134696 [details]
make.log


>To be entirely clear about this, what I'm currently proposing is git master + >Attachment #134544 [details] + Attachment #134565 [details] + Attachment #134546 [details] >[details] (in that order).`
I did that and did a fresh rebuild

I copied the generated libdbus-1-3.dll to a windows host and inspected with explorer->properties->details if a versioninfo has been added - it is not present.
Comment 39 Ralf Habacker 2017-10-05 22:42:36 UTC
(In reply to Ralf Habacker from comment #38)
> Created attachment 134696 [details]
> make.log
> 
> 
> >To be entirely clear about this, what I'm currently proposing is git master + >Attachment #134544 [details] + Attachment #134565 [details] + Attachment #134546 [details] >[details] (in that order).`
> I did that and did a fresh rebuild
> 
> I copied the generated libdbus-1-3.dll to a windows host and inspected with
> explorer->properties->details if a versioninfo has been added - it is not
> present.
Looks strange, opening libdbus-1-3.dll with ResourceEditor show the version info resource entry, adding the column "file version" to explorer in detail view, which normally shows that version, is empty.
Comment 40 Ralf Habacker 2017-10-05 23:05:52 UTC
(In reply to Ralf Habacker from comment #39)
> (In reply to Ralf Habacker from comment #38)
> > Created attachment 134696 [details]
> > make.log
> > 
> > 
> > >To be entirely clear about this, what I'm currently proposing is git master + >Attachment #134544 [details] + Attachment #134565 [details] + Attachment #134546 [details] >[details] (in that order).`
> > I did that and did a fresh rebuild
> > 
> > I copied the generated libdbus-1-3.dll to a windows host and inspected with
> > explorer->properties->details if a versioninfo has been added - it is not
> > present.
> Looks strange, opening libdbus-1-3.dll with ResourceEditor show the version
> info resource entry, adding the column "file version" to explorer in detail
> view, which normally shows that version, is empty.

https://stackoverflow.com/questions/852568/version-resource-in-dll-not-visible-with-right-click

According to https://msdn.microsoft.com/en-us/library/windows/desktop/aa381058(v=vs.85).aspx I also suggest a view changes to versioninfo.rc.in for usage in dbus-1-3.dll
 FILEOS 0x00040000L
 FILETYPE 0x2L

for usage in applications 
 FILETYPE 0x1L

Also I saw that cmake build system only adds versioninfo for msvc - it works also with gcc.
Comment 41 Simon McVittie 2017-10-06 11:52:17 UTC
(In reply to Ralf Habacker from comment #39)
> Looks strange, opening libdbus-1-3.dll with ResourceEditor show the version
> info resource entry, adding the column "file version" to explorer in detail
> view, which normally shows that version, is empty.

OK, that sounds as though the build system is correctly linking versioninfo.o into the library (are you testing with or without my patches from this bug?), but the content of versioninfo.rc isn't what Windows wants to see. So I think the build-system parts of whichever version you tested are OK?

Attachment #134696 [details] shows .libs/versioninfo.o correctly being linked into the DLL, similar to what I quoted in Comment #35.

(In reply to Ralf Habacker from comment #40)
> https://stackoverflow.com/questions/852568/version-resource-in-dll-not-visible-with-right-click

If one of the changes suggested there fixes this for you, please propose a patch with a commit message justifying it.

> According to
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa381058(v=vs.85).aspx
> I also suggest a view changes to versioninfo.rc.in for usage in
> dbus-1-3.dll
>  FILEOS 0x00040000L
>  FILETYPE 0x2L

Sounds good, please propose a patch with a commit message. FILETYPE VFT_DLL (or its numeric value if that's more reliable/portable), and any reasonable FILEOS value that represents 32-bit Windows NT, seem like reasonable things to use. The FILEOS is currently set to 0x40004L - I don't know what that represents.

If we continue to use raw numeric values for "magic numbers" in the versioninfo resource, I'd prefer to at least have a comment explaining what the magic number means, similar to the comment you added to the CMake code that generates disable-uac.rc. Unfortunately, the original version of versioninfo doesn't have those.

It would be even better if the numeric constants could be replaced with symbolic macros like VFT_DLL by #include'ing some suitable header, so future maintainers can easily cross-reference them with Microsoft documentation.

> for usage in applications 
>  FILETYPE 0x1L

We don't currently set version info in any executables. If there's some reason why we should, go ahead.

I'd prefer to get the patches on this bug applied first, if they're good. After that, adding version info to any other executables should use the same pattern for invoking windres that we use in tools/.

> Also I saw that cmake build system only adds versioninfo for msvc - it works
> also with gcc.

Please propose a patch if you would like this to change.

Some of the CMake stuff around versioninfo seems really weird:

    file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/afxres.h "")
    set_source_files_properties(versioninfo.rc COMPILE_FLAGS "-D__LINE__=1")

Why are those necessary?

For the first one, I don't see any other mentions of afxres.h in the dbus source tree - git history says you removed it in commit e3a14eb, for Bug #96181. So we probably don't need to create an empty header file any more?

For the second, I thought .rc files were passed through a C preprocessor that should already replace __LINE__ with the real line number? But if not, deleting the "#line" directive from dbus/versioninfo.rc.in seems a cleaner fix than pretending __LINE__ is always at line 1?

You know more about CMake and Windows than I do, so perhaps there's some reason for those lines that I don't understand; but if they aren't needed, we should probably remove them.
Comment 42 Simon McVittie 2017-10-06 12:01:52 UTC
Created attachment 134703 [details] [review]
UNTESTED: cmake: Stop creating an empty afxres.h

The resource file used to #include this, but it was unnecessary,
and Ralf removed it in commit e3a14eb.

---

I don't use MSVC, so I can't test this. If you want to apply it, please test on MSVC first, then remove the UNTESTED: prefix from the commit message :-)
Comment 43 Simon McVittie 2017-10-06 12:03:52 UTC
Created attachment 134704 [details] [review]
UNTESTED: Windows: Stop manipulating line numbering in versioninfo.rc

If __LINE__ doesn't work in MSVC's resource compiler, then removing
the #line directive altogether seems a simpler fix than redefining
__LINE__ to the wrong value.

---

Again, this needs testing on MSVC as well as review. It doesn't seem to break the build with mingw, but that's the limit of my ability to test this.
Comment 44 Ralf Habacker 2017-10-06 20:48:27 UTC
Created attachment 134716 [details] [review]
Update versioninfo.rc.in

- let versioninfo be visible in explorer by adding a "Translation" value
- use constants
- fix strings

BUG:
Comment 45 Ralf Habacker 2017-10-06 21:07:18 UTC
Comment on attachment 134716 [details] [review]
Update versioninfo.rc.in

will be superseeded
Comment 46 Ralf Habacker 2017-10-06 21:08:46 UTC
Created attachment 134719 [details] [review]
- match StringFileInfo with VarFileInfo
Comment 47 Ralf Habacker 2017-10-06 21:11:31 UTC
(In reply to Simon McVittie from comment #41)
> (In reply to Ralf Habacker from comment #39)
> > Looks strange, opening libdbus-1-3.dll with ResourceEditor show the version
> > info resource entry, adding the column "file version" to explorer in detail
> > view, which normally shows that version, is empty.
> 
> OK, that sounds as though the build system is correctly linking
> versioninfo.o into the library (are you testing with or without my patches
> from this bug?), 

with patches in the order mentioned at comment 34

> but the content of versioninfo.rc isn't what Windows wants
> to see. So I think the build-system parts of whichever version you tested
> are OK?

yes, I think so

> Attachment #134696 [details] shows .libs/versioninfo.o correctly being
> linked into the DLL, similar to what I quoted in Comment #35.
yes

<snip>
> I'd prefer to get the patches on this bug applied first, if they're good.
sure

> After that, adding version info to any other executables should use the same
> pattern for invoking windres that we use in tools/.
 
> > Also I saw that cmake build system only adds versioninfo for msvc - it works
> > also with gcc.
> 
> Please propose a patch if you would like this to change.
> 
> Some of the CMake stuff around versioninfo seems really weird:
> 
>     file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/afxres.h "")
>     set_source_files_properties(versioninfo.rc COMPILE_FLAGS "-D__LINE__=1")
> 
> Why are those necessary?
I don't know, msvc also defines it (see https://msdn.microsoft.com/en-us/library/b0084kay%28VS.80%29.aspx)
> 
> For the first one, I don't see any other mentions of afxres.h in the dbus
> source tree - git history says you removed it in commit e3a14eb, for Bug
> #96181. So we probably don't need to create an empty header file any more?
it looks so

> For the second, I thought .rc files were passed through a C preprocessor
> that should already replace __LINE__ with the real line number?
msvc should do so, see above
> But if not deleting the "#line" directive from dbus/versioninfo.rc.in seems
> a cleaner fix than pretending __LINE__ is always at line 1?
yes

> You know more about CMake and Windows than I do, so perhaps there's some
> reason for those lines that I don't understand; but if they aren't needed,
> we should probably remove them.
let us fix the autotools stuff first, then I will take care about the cmake side - we should update the bug title then ?
Comment 48 Ralf Habacker 2017-10-06 21:28:33 UTC
(In reply to Ralf Habacker from comment #47)
> > Some of the CMake stuff around versioninfo seems really weird:
> > 
> >     file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/afxres.h "")
Looking at the related commit let me think that the dummy afxres.h was required   because afxres.h has been referenced in versioninfo.rc.in file. 

> >     set_source_files_properties(versioninfo.rc COMPILE_FLAGS "-D__LINE__=1")
> > 
> > Why are those necessary?
> I don't know, msvc also defines it (see
> https://msdn.microsoft.com/en-us/library/b0084kay%28VS.80%29.aspx)

There must be a reason why this has been defined - I guess the windows rc tool did not set __LINE__ and does not care about the #line statement, so the value is only a dummy. Because the #line statement was in the rc.in file, probably to sync line references in error messages, it was required.

- with the #line statement

~/src/dbus-cmake-cross-x86-build/dbus> cpp versioninfo.rc 
# 1 "versioninfo.rc"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "versioninfo.rc"
# 15 "versioninfo.rc.in"
versioninfo.rc.in:17:21: fatal error: windows.h: Datei oder Verzeichnis nicht gefunden
compilation terminated.

- without

:~/src/dbus-cmake-cross-x86-build/dbus> cpp versioninfo.rc 
# 1 "versioninfo.rc"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "versioninfo.rc"
versioninfo.rc:18:21: fatal error: windows.h: Datei oder Verzeichnis nicht gefunden
 #include <windows.h>

ms supports my assumption, see https://msdn.microsoft.com/de-de/library/windows/desktop/aa381032(v=vs.85).aspx
Comment 49 Simon McVittie 2017-10-09 10:26:13 UTC
(In reply to Ralf Habacker from comment #48)
> (In reply to Ralf Habacker from comment #47)
> > > Some of the CMake stuff around versioninfo seems really weird:
> > > 
> > >     file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/afxres.h "")
> Looking at the related commit let me think that the dummy afxres.h was
> required   because afxres.h has been referenced in versioninfo.rc.in file. 

Right, but then you removed that reference in commit e3a14eb, so it shouldn't be necessary any more.

Please consider Attachment #134703 [details]. If you test it with at least CMake+MSVC (CMake+mingw is tested on travis-ci already) and it works, we can apply it.
Comment 50 Simon McVittie 2017-10-09 10:31:17 UTC
(In reply to Ralf Habacker from comment #48)
> > >     set_source_files_properties(versioninfo.rc COMPILE_FLAGS "-D__LINE__=1")
> > > 
> > > Why are those necessary?
> > I don't know, msvc also defines it (see
> > https://msdn.microsoft.com/en-us/library/b0084kay%28VS.80%29.aspx)

I know that __LINE__ is a predefined macro in an ISO C cpp implementation. All ISO C preprocessors have to implement it, including GNU cpp (part of gcc) and MSVC.

What is less clear to me is whether the Windows rc compiler passes rc files through an ISO C preprocessor as part of compilation. Examples on MSVC that include various Windows headers suggest that it does?

> probably to sync line references in error messages
...
> versioninfo.rc.in:17:21: fatal error: windows.h: Datei oder Verzeichnis
> nicht gefunden
...
> versioninfo.rc:18:21: fatal error: windows.h: Datei oder Verzeichnis nicht
> gefunden

I don't think this difference is important enough to justify hacks like redefining __LINE__, to be honest.

If __LINE__ works with its intended meaning (a magic macro that expands to the line number where it appeared) on all the rc compilers we support (gcc/mingw windres, and whatever MSVC uses), then we can keep the #line directive and remove the hack that redefines __LINE__ from the CMake build system. Otherwise, I think we should remove both (Attachment #134704 [details]).
Comment 51 Simon McVittie 2017-10-09 10:33:08 UTC
(In reply to Ralf Habacker from comment #47)
> (In reply to Simon McVittie from comment #41)
> > OK, that sounds as though the build system is correctly linking
> > versioninfo.o into the library (are you testing with or without my patches
> > from this bug?), 
> 
> with patches in the order mentioned at comment 34

Great. So those three patches work, and are ready for merge as soon as they have a positive review. Do you want to review them, or are you happy with Philip's positive reviews?
Comment 52 Simon McVittie 2017-10-09 11:00:54 UTC
Comment on attachment 134719 [details] [review]
- match StringFileInfo with VarFileInfo

Review of attachment 134719 [details] [review]:
-----------------------------------------------------------------

I have some queries, but this is certainly going in the right direction.

::: configure.ac
@@ +146,4 @@
>      # Yes, on Windows it really does work like this.
>      # http://support.microsoft.com/kb/111855
>      AC_DEFINE(FD_SETSIZE,8192,[The maximum number of connections that can be handled at once])
> +    BUILD_TIMESTAMP=`date --iso-8601=minutes | sed ':a;N;$!ba;s/\n//g'`

What is this sed incantation doing, and can we do it in a simpler way?

If I'm reading it correctly, it's:

           # implicitly: for each line (because that's what sed does)
:a         # label: a
N          # append next line to pattern space
$! ba      # if not at last line, goto a
s/\n//g    # remove all newlines
           # implicitly: print pattern space (because sed -n was not used)

But I don't understand why this is necessary. `date --iso-8601=minutes` outputs a single line like

2017-10-09T11:51+01:00

and the trailing newline is removed by the `` operator. So the sed snippet should never have any practical effect?

::: dbus/versioninfo.rc.in
@@ +20,5 @@
>  VS_VERSION_INFO VERSIONINFO
>   FILEVERSION @BUILD_FILEVERSION@
>   PRODUCTVERSION @BUILD_FILEVERSION@
>   FILEFLAGSMASK 0x3fL
> + FILEFLAGSMASK 0x3fL

Is this line duplicated by mistake, or is there some reason for it?

I'm not sure I understand why this field even exists - why would a DLL ever have flags in FILEFLAGS in an undefined state? - but if it's what you need to do, then include it.

@@ +27,2 @@
>  #else
> + FILEFLAGS 0x0L

What was flag 0x20? I can't give a meaningful review of its removal without knowing what it does. It should be mentioned in the commit message as something like:

> Stop setting flag 0x20 (VS_FF_WHATEVER), which is not appropriate here because [some reason]

Assuming VS_FF_DEBUG is 0x01, I like this version (with the symbolic constant) much better than before.

@@ +29,4 @@
>  #endif
> + FILEOS VOS__WINDOWS32
> + FILETYPE VFT_DLL
> + FILESUBTYPE VFT2_UNKNOWN

What is 0x40004L as a FILEOS? Is it VOS__WINDOWS32 or something different?

The commit message should say something like

> Change FILEOS from VOS__WHATEVER to VOS__WINDOWS32

unless VOS__WINDOWS32 *is* 0x40004L.

Changing FILETYPE to VFT_DLL is clearly correct.

Changing FILESUBTYPE to VFT2_UNKNOWN seems a bit odd, because meanings for the VFT2 constants aren't defined for VFT_DLL. The MSDN documentation says "The subtype parameter is zero unless the filetype parameter in the FILETYPE statement is VFT_DRV, VFT_FONT, or VFT_VXD", so I would be inclined to leave it as FILESUBTYPE 0x0L? But if VFT2_UNKNOWN is numerically zero, as I suspect it is, you can use VFT2_UNKNOWN if you prefer.

@@ +33,5 @@
>  BEGIN
>      BLOCK "StringFileInfo"
>      BEGIN
> +        /* string need to match concated hex values in 'VarFileInfo' block */
> +        BLOCK "040904e4"

OK: this is declaring that the next block is in English (0x409) encoded in Windows-1252 (0x4e4 == 1252), if I understand correctly

@@ +38,2 @@
>          BEGIN
> +            VALUE "Comments", "Provided under the terms of the GNU Lesser General Public License >= 2.0 or Academic Free License version 2.1\0"

OK

@@ +38,3 @@
>          BEGIN
> +            VALUE "Comments", "Provided under the terms of the GNU Lesser General Public License >= 2.0 or Academic Free License version 2.1\0"
> +            VALUE "CompanyName", "freedesktop.org\0"

We aren't a company, but I suppose this is close enough; we're an organisation, which is the next best thing

@@ +41,3 @@
>              VALUE "FileDescription", "dbus - FreeDesktop message bus system\0"
>              VALUE "FileVersion", "@DBUS_VERSION@\0"
> +            VALUE "InternalName", "libdbus-1-3\0"

OK

@@ +41,4 @@
>              VALUE "FileDescription", "dbus - FreeDesktop message bus system\0"
>              VALUE "FileVersion", "@DBUS_VERSION@\0"
> +            VALUE "InternalName", "libdbus-1-3\0"
> +            VALUE "LegalCopyright", "Copyright © 2002-2017 freedesktop.org\0"

Not your fault, but this is not true. freedesktop.org doesn't exist as a legal entity that can hold copyright. The copyright on dbus is held by the various contributors (including you) or their employers (including my employer, Collabora).

I think the best we can do in a one-line metadata field would be something like

VALUE "LegalCopyright", "Copyright © 1994-2017 dbus contributors, see dbus source code for details\0"

because something like https://anonscm.debian.org/cgit/pkg-utopia/dbus.git/tree/debian/copyright is far too large to put in DLL metadata.

@@ +45,2 @@
>              VALUE "LegalTrademarks", "\0"
> +            VALUE "OriginalFilename", "libdbus-1-3.dll\0"

OK

@@ +52,5 @@
> +    BLOCK "VarFileInfo"
> +    BEGIN
> +        /* supports English language (0x409) in the Windows ANSI codepage (1252). */
> +        VALUE "Translation", 0x409, 1252
> +    END

OK, and thanks for including the comment!
Comment 53 Ralf Habacker 2017-10-09 11:24:14 UTC
Comment on attachment 134544 [details] [review]
Windows: Simplify compiling versioninfo.rc by using  libtool facilities

Review of attachment 134544 [details] [review]:
-----------------------------------------------------------------

Looks good to me - This patch makes rc file support for shared libraries much easier to read 

> libtool has built-in support for Windows resources, and we even
> enable it in configure.ac. What it doesn't have is a built-in rule
> for generating Libtool objects using that built-in support, but
> we can add one.

Looks that libtool is incomplete in this area - Should this fix not be part of further libtool releases to avoid hacking any libtool client package ?
Comment 54 Ralf Habacker 2017-10-09 11:24:51 UTC
Comment on attachment 134546 [details] [review]
Windows: Check for $RC, not $WINDRES

Review of attachment 134546 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 55 Simon McVittie 2017-10-09 12:12:55 UTC
(In reply to Ralf Habacker from comment #53)
> > libtool has built-in support for Windows resources, and we even
> > enable it in configure.ac. What it doesn't have is a built-in rule
> > for generating Libtool objects using that built-in support, but
> > we can add one.
> 
> Looks that libtool is incomplete in this area - Should this fix not be part
> of further libtool releases to avoid hacking any libtool client package ?

I agree that having this happen automatically would be good, and if that isn't feasible for some reason, having it documented would also be good. I don't develop on Windows, so I'm really not the right person to be proposing patches for that.

If the feature request is "add rules so Windows resources work just like C source", then I think it would have to be fixed in Automake rather than libtool, but I could be wrong (how Autoconf, Automake and libtool fit together is not always clear).

The next best thing would be for this to be documented, and there is already a bug report against libtool asking for that: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25914
Comment 56 Simon McVittie 2017-10-09 12:30:56 UTC
(In reply to Simon McVittie from comment #55)
> The next best thing would be for this to be documented, and there is already
> a bug report against libtool asking for that:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25914

I've replied to that bug report with some notes on what we did here.
Comment 57 Simon McVittie 2017-10-09 12:33:21 UTC
(In reply to Ralf Habacker from comment #54)
> Comment on attachment 134546 [details] [review]
> Windows: Check for $RC, not $WINDRES
...
> looks good

Thanks. Applying this one is blocked by Attachment #134565 [details]: we can't stop setting WINDRES until tools/Makefile.am has stopped using it.
Comment 58 Simon McVittie 2017-10-09 12:37:55 UTC
Comment on attachment 134544 [details] [review]
Windows: Simplify compiling versioninfo.rc by using  libtool facilities

Applied for 1.11.22, commit 7250c73e
Comment 59 Ralf Habacker 2017-10-17 14:43:42 UTC
Comment on attachment 134565 [details] [review]
Windows: Use libtool-detected RC to compile resources in  tools/

Review of attachment 134565 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 60 Ralf Habacker 2017-10-17 14:46:27 UTC
Created attachment 134882 [details] [review]
Update versioninfo.rc.in

- let versioninfo be visible in explorer by adding a "Translation" value
- change FILEOS from VOS_NT_WINDOWS32 to VOS__WINDOWS32
- stop setting FILEFLAGS 0x20 (VS_FF_SPECIALBUILD), which is not
  appropriate here because we build the normal version, not a special
  version
- use constants
- fix strings

Bug:
Comment 61 Ralf Habacker 2017-10-17 14:59:16 UTC
Created attachment 134883 [details] [review]
Update versioninfo.rc.in

- include <windows.h> to be able to use constants
- let versioninfo be visible in explorer by adding a "Translation" value
- change FILEOS from VOS_NT_WINDOWS32 to VOS__WINDOWS32
- stop setting FILEFLAGS 0x20 (VS_FF_SPECIALBUILD), which is not
  appropriate here because we build the normal version, not a special
  version
- use constants
- fix strings
Comment 62 Ralf Habacker 2017-10-17 18:18:05 UTC
Comment on attachment 134703 [details] [review]
UNTESTED: cmake: Stop creating an empty afxres.h

Review of attachment 134703 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 63 Ralf Habacker 2017-10-17 18:18:20 UTC
Comment on attachment 134704 [details] [review]
UNTESTED: Windows: Stop manipulating line numbering in versioninfo.rc

Review of attachment 134704 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 64 Ralf Habacker 2017-10-17 18:25:26 UTC
Comment on attachment 134703 [details] [review]
UNTESTED: cmake: Stop creating an empty afxres.h

tested with msvc and applied to master
Comment 65 Ralf Habacker 2017-10-17 18:25:43 UTC
Comment on attachment 134704 [details] [review]
UNTESTED: Windows: Stop manipulating line numbering in versioninfo.rc

test with msvc and applied to master
Comment 66 Ralf Habacker 2017-10-17 19:09:55 UTC
Created attachment 134885 [details] [review]
Use cmake build in timestamp function to generate the build time stamp

The recent implementation generates a timestamp containing eol on
linux hosts, which generates unparseable versioninfo.rc.

This commit raise the minimal required cmake version to 3.0.2.

Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Bug:
Comment 67 Ralf Habacker 2017-10-17 19:10:14 UTC
Created attachment 134886 [details] [review]
Add version info to dbus-1 target for non msvc builds on Windows too

Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Bug:
Comment 68 Ralf Habacker 2017-10-17 19:11:16 UTC
Created attachment 134887 [details] [review]
- update commit message

- skip timestamp generating patch
Comment 69 Simon McVittie 2017-10-18 11:48:02 UTC
(In reply to Ralf Habacker from comment #59)
> Windows: Use libtool-detected RC to compile resources in  tools/
> 
> Review of attachment 134565 [details] [review]
> -----------------------------------------------------------------
> 
> looks good

Thanks, pushed for 1.11.22 (or perhaps 1.12.0).
Comment 70 Simon McVittie 2017-10-18 11:48:30 UTC
(In reply to Ralf Habacker from comment #54)
> Windows: Check for $RC, not $WINDRES
> 
> Review of attachment 134546 [details] [review]:
> -----------------------------------------------------------------
> 
> looks good

Pushed for 1.11.22 or 1.12.0
Comment 71 Simon McVittie 2017-10-18 11:52:06 UTC
Comment on attachment 134885 [details] [review]
Use cmake build in timestamp function to generate the build time stamp

Review of attachment 134885 [details] [review]:
-----------------------------------------------------------------

This looks OK in principle but please don't push it right now.

::: cmake/CMakeLists.txt
@@ +7,4 @@
>  project(dbus)
>  
>  # we need to be up to date
> +CMAKE_MINIMUM_REQUIRED(VERSION 3.0.2 FATAL_ERROR)

Unfortunately this is going to break Travis-CI, since that's stuck on Ubuntu 14.04 which only has CMake 2.8.12.2. We can move the CMake builds into a Docker container running a less ancient Debian or Ubuntu to get a newer CMake, although that will slow things down.
Comment 72 Simon McVittie 2017-10-18 11:53:55 UTC
Comment on attachment 134886 [details] [review]
Add version info to dbus-1 target for non msvc builds on Windows too

Review of attachment 134886 [details] [review]:
-----------------------------------------------------------------

Looks fine, although to avoid breaking CI we probably want to delay applying it until your timestamp change is in, for which we need to move CMake builds into a Docker container because Travis-CI is stuck in 2014. (https://github.com/travis-ci/travis-ci/issues/5821)
Comment 73 Simon McVittie 2017-10-18 11:59:28 UTC
Comment on attachment 134887 [details] [review]
- update commit message

Review of attachment 134887 [details] [review]:
-----------------------------------------------------------------

> change FILEOS from VOS_NT_WINDOWS32 to VOS__WINDOWS32

Is there any particular reason to prefer VOS__WINDOWS32 over VOS_NT_WINDOWS32?

> stop setting FILEFLAGS 0x20 (VS_FF_SPECIALBUILD), which is not
> appropriate here because we build the normal version, not a special
> version

OK, that seems good.
Comment 74 Ralf Habacker 2017-10-18 12:03:29 UTC
(In reply to Simon McVittie from comment #71)
> Comment on attachment 134885 [details] [review] [review]
> Use cmake build in timestamp function to generate the build time stamp
> 
> Review of attachment 134885 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This looks OK in principle but please don't push it right now.
> 
> ::: cmake/CMakeLists.txt
> @@ +7,4 @@
> >  project(dbus)
> >  
> >  # we need to be up to date
> > +CMAKE_MINIMUM_REQUIRED(VERSION 3.0.2 FATAL_ERROR)
> 
> Unfortunately this is going to break Travis-CI, since that's stuck on Ubuntu
> 14.04 which only has CMake 2.8.12.2. We can move the CMake builds into a
> Docker container running a less ancient Debian or Ubuntu to get a newer
> CMake, although that will slow things down.

Another options would be to remove the eol appended to the return value of the unix variant of the timestamp function, which is used on cross compiling and breaks versioninfo.rc file format.

Using the buildin timestamp function looks better at a first look.
Comment 75 Simon McVittie 2017-10-18 12:07:59 UTC
(In reply to Simon McVittie from comment #71)
> Unfortunately this is going to break Travis-CI

No, my mistake, it looks like Travis-CI has a newer CMake than its Ubuntu base OS would normally come with (3.2.2 for the Ubuntu 14.04 machines we're using, according to https://docs.travis-ci.com/user/languages/cpp/).

So those three patches are probably all fine. My only query is why you're preferring VOS__WINDOWS32 over VOS_NT_WINDOWS32 since they would seem to me to be equivalent - whatever that reason is, please mention it briefly in the commit message. (If the reason is "everyone else uses VOS__WINDOWS32" then that would be unsatisfying but OK.)
Comment 76 Ralf Habacker 2017-10-18 12:26:57 UTC
(In reply to Simon McVittie from comment #73)
> Comment on attachment 134887 [details] [review] [review]
> - update commit message
> 
> Review of attachment 134887 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> > change FILEOS from VOS_NT_WINDOWS32 to VOS__WINDOWS32
> 
> Is there any particular reason to prefer VOS__WINDOWS32 over
> VOS_NT_WINDOWS32?

yes, https://msdn.microsoft.com/en-us/library/windows/desktop/ms646997(v=vs.85).aspx mentions  "VOS_NT_WINDOWS32 0x00040004L The file was designed for Windows NT."

I will update the commit message, thanks for this pointer.
Comment 77 Simon McVittie 2017-10-18 12:27:53 UTC
Testing your patches on Travis-CI at <https://travis-ci.org/smcv/dbus/builds/289482208>.

If that doesn't work, we can move the two CMake builds into a Docker container by editing .travis.yml, but that'll take longer to test each new commit (you'll probably notice the three Docker builds already present at the end of the list take disproportionately long, because they have to download a whole container before they can start work).
Comment 78 Ralf Habacker 2017-10-18 12:29:03 UTC
(In reply to Simon McVittie from comment #73)
> Comment on attachment 134887 [details] [review] [review]
> - update commit message
> 
> Review of attachment 134887 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> > change FILEOS from VOS_NT_WINDOWS32 to VOS__WINDOWS32
> 
> Is there any particular reason to prefer VOS__WINDOWS32 over
> VOS_NT_WINDOWS32?

yes, https://msdn.microsoft.com/en-us/library/windows/desktop/ms646997(v=vs.85).aspx mentions  "VOS_NT_WINDOWS32 0x00040004L The file was designed for Windows NT."

I will update the commit message, thanks for this pointer.
Comment 79 Ralf Habacker 2017-10-18 12:33:43 UTC
(In reply to Ralf Habacker from comment #78)
> I will update the commit message, thanks for this pointer.
sorry for double posting
Comment 80 Ralf Habacker 2017-10-18 12:37:20 UTC
Created attachment 134909 [details] [review]
Update versioninfo.rc.in

- include <windows.h> to be able to use constants
- let versioninfo be visible in explorer by adding a "Translation" value
- change FILEOS from VOS_NT_WINDOWS32, which was intended for Windows NT,
  to VOS__WINDOWS32
- stop setting FILEFLAGS 0x20 (VS_FF_SPECIALBUILD), which is not
  appropriate here because we build the normal version, not a special
  version
- use constants
- fix strings
Comment 81 Simon McVittie 2017-10-18 12:43:56 UTC
(In reply to Ralf Habacker from comment #76)
> (In reply to Simon McVittie from comment #73)
> > Is there any particular reason to prefer VOS__WINDOWS32 over
> > VOS_NT_WINDOWS32?
> 
> yes,
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms646997(v=vs.85).
> aspx mentions  "VOS_NT_WINDOWS32 0x00040004L The file was designed for
> Windows NT."

All Windows versions since XP have been NT-derived and we no longer support anything older than XP, so I'm not sure how meaningful it is to distinguish... but if VOS_NT_WINDOWS32 is conventionally used to mean the "professional" branch (NT 4, etc.) that merged with the "consumer" branch with the release of XP, and VOS__WINDOWS32 is conventionally used to mean any version of consumer Windows, I have no objection to changing it to VOS__WINDOWS32.

Out of interest, do we still aim to support XP or did you get rid of that? If you want to remove support for older Windows or MSVC versions, just after I branch off dbus-1.12 would be an excellent time for that (and in particular if there is still XP support we should probably remove it). If we do that sort of cleanup shortly after 1.12.0, then 1.12.x will still be there for a while for people with obsolete OSs or toolchains.
Comment 82 Simon McVittie 2017-10-18 12:45:30 UTC
Comment on attachment 134885 [details] [review]
Use cmake build in timestamp function to generate the build time stamp

Review of attachment 134885 [details] [review]:
-----------------------------------------------------------------

Seems to work. Ship it!
Comment 83 Simon McVittie 2017-10-18 12:46:11 UTC
Comment on attachment 134886 [details] [review]
Add version info to dbus-1 target for non msvc builds on Windows too

Review of attachment 134886 [details] [review]:
-----------------------------------------------------------------

Go ahead (after merging the timestamp fix).
Comment 84 Simon McVittie 2017-10-18 12:47:32 UTC
Comment on attachment 134909 [details] [review]
Update versioninfo.rc.in

Review of attachment 134909 [details] [review]:
-----------------------------------------------------------------

Looks fine except for:

::: configure.ac
@@ +146,4 @@
>      # Yes, on Windows it really does work like this.
>      # http://support.microsoft.com/kb/111855
>      AC_DEFINE(FD_SETSIZE,8192,[The maximum number of connections that can be handled at once])
> +    BUILD_TIMESTAMP=`date --iso-8601=minutes | sed ':a;N;$!ba;s/\n//g'`

Wait, why has this bit come back?

I would prefer to leave it as `date --iso-8601=minutes` unless you can explain what was wrong with that version.
Comment 85 Ralf Habacker 2017-10-18 13:10:21 UTC
(In reply to Simon McVittie from comment #84)
> Wait, why has this bit come back?

comes from a previous version, seems that git bz apply did not updated correctly
> I would prefer to leave it as `date --iso-8601=minutes` unless you can
> explain what was wrong with that version.
see above, i will remove it. 

While looking again into this stuff I found differences in the format of the generated time stamp.

autotools    VALUE "SpecialBuild", "2017-10-18T15:05+02:00\0"
cmake        VALUE "SpecialBuild", "201710180845\0"
Comment 86 Simon McVittie 2017-10-18 13:27:40 UTC
(In reply to Ralf Habacker from comment #85)
> While looking again into this stuff I found differences in the format of the
> generated time stamp.
> 
> autotools    VALUE "SpecialBuild", "2017-10-18T15:05+02:00\0"
> cmake        VALUE "SpecialBuild", "201710180845\0"

You can use `date "+%Y%m%d%H%M"` (I think that matches?) instead of `date --iso=minutes` if you prefer CMake's format, or conversely, adjust CMake's format string if you prefer the longer form. I don't think it really matters either way - this is just arbitrary text as far as the build system is concerned, if I understand correctly?

https://www.unix.com/man-page/posix/1p/date/ documents the portable subset of `date` syntax (which I notice we're not actually using - but probably nobody is going to compile dbus for Windows on anything other than Windows or Linux so it doesn't really matter).
Comment 87 Ralf Habacker 2017-10-18 13:53:16 UTC
Created attachment 134910 [details] [review]
Update versioninfo.rc.in

- include <windows.h> to be able to use constants
- let versioninfo be visible in explorer by adding a "Translation" value
- change FILEOS from VOS_NT_WINDOWS32, which was intended for Windows NT,
  to VOS__WINDOWS32
- stop setting FILEFLAGS 0x20 (VS_FF_SPECIALBUILD), which is not
  appropriate here because we build the normal version, not a special
  version
- use constants
- fix strings

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=103015
Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Comment 88 Ralf Habacker 2017-10-18 13:53:58 UTC
Created attachment 134911 [details] [review]
Fix cmake 3.5 configure error on opening a non existant file

Previous cmake versions seemed to be more tolerant.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=103015
Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Comment 89 Simon McVittie 2017-10-18 16:13:05 UTC
Comment on attachment 134910 [details] [review]
Update versioninfo.rc.in

Review of attachment 134910 [details] [review]:
-----------------------------------------------------------------

Looks good, go ahead
Comment 90 Simon McVittie 2017-10-18 16:13:43 UTC
Comment on attachment 134911 [details] [review]
Fix cmake 3.5 configure error on opening a non existant file

Review of attachment 134911 [details] [review]:
-----------------------------------------------------------------

Sure, go ahead
Comment 91 Ralf Habacker 2017-10-18 21:20:39 UTC
Created attachment 134913 [details] [review]
Add version info to installed executables for cmake build system on Windows

Bug:
Comment 92 Simon McVittie 2017-10-19 11:17:29 UTC
Comment on attachment 134913 [details] [review]
Add version info to installed executables for cmake build system on Windows

Review of attachment 134913 [details] [review]:
-----------------------------------------------------------------

Don't you want to change "FILETYPE VFT_DLL" to something more appropriate for the executables?

::: cmake/tools/CMakeLists.txt
@@ +80,5 @@
>  if(WIN32)
> +    set(DBUS_INTERNAL_NAME "dbus-update-activation-environment")
> +    set(DBUS_ORIGIN_INTERNAL_NAME "${DBUS_INTERNAL_NAME}${CMAKE_EXECUTABLE_SUFFIX}")
> +    configure_file(${CMAKE_SOURCE_DIR}/../dbus/versioninfo.rc.in ${CMAKE_CURRENT_BINARY_DIR}/versioninfo-${DBUS_INTERNAL_NAME}.rc)
> +    list(APPEND dbus_update_activation_environment_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/versioninfo-${DBUS_INTERNAL_NAME}.rc)

Is it valid to embed multiple resources in the same .exe? Does it do what you would hope it does?

::: configure.ac
@@ +151,3 @@
>      # Assume DBUS_VERSION is always three numbers
>      BUILD_FILEVERSION=`echo "$DBUS_VERSION" | sed -e 's/\./,/g'`,0
>      AC_SUBST(BUILD_FILEVERSION)

I think this deserves a comment:

In the CMake build system we generate multiple files, versioninfo-*.rc, with a 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.
Comment 93 Ralf Habacker 2017-10-19 17:58:05 UTC
Created attachment 134924 [details] [review]
Add version info to installed executables for cmake build system on Windows

Bug:
Comment 94 Ralf Habacker 2017-10-19 18:05:18 UTC
(In reply to Simon McVittie from comment #92)
> Comment on attachment 134913 [details] [review] [review]
> Add version info to installed executables for cmake build system on Windows
> 
> Review of attachment 134913 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Don't you want to change "FILETYPE VFT_DLL" to something more appropriate
> for the executables?
make sense

> ::: cmake/tools/CMakeLists.txt
> @@ +80,5 @@
> >  if(WIN32)
> > +    set(DBUS_INTERNAL_NAME "dbus-update-activation-environment")
> > +    set(DBUS_ORIGIN_INTERNAL_NAME "${DBUS_INTERNAL_NAME}${CMAKE_EXECUTABLE_SUFFIX}")
> > +    configure_file(${CMAKE_SOURCE_DIR}/../dbus/versioninfo.rc.in ${CMAKE_CURRENT_BINARY_DIR}/versioninfo-${DBUS_INTERNAL_NAME}.rc)
> > +    list(APPEND dbus_update_activation_environment_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/versioninfo-${DBUS_INTERNAL_NAME}.rc)
> 
> Is it valid to embed multiple resources in the same .exe? 
yes, they use different types

> Does it do what you would hope it does?
yes 
> 
> ::: configure.ac
> @@ +151,3 @@
> >      # Assume DBUS_VERSION is always three numbers
> >      BUILD_FILEVERSION=`echo "$DBUS_VERSION" | sed -e 's/\./,/g'`,0
> >      AC_SUBST(BUILD_FILEVERSION)
> 
> I think this deserves a comment:
> In the CMake build system we generate multiple files, versioninfo-*.rc, with
> a 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.

automake probably can have same support for executable ?
I know that generated files need to be added to AC_CONFIG_FILES in configure.ac, but I do not know how to provide different variables to them.
Comment 95 Simon McVittie 2017-10-20 11:47:41 UTC
(In reply to Ralf Habacker from comment #94)
> > I think this deserves a comment:
> > In the CMake build system we generate multiple files, versioninfo-*.rc, with
> > a 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.
> 
> automake probably can have same support for executable ?

It could, but I'm not sure how worth the effort it would be. How important is this version info for executables? There's a limit to how important it can be if we've had an official Windows port for 10 years without needing it? :-)

I don't think a differing level of version-info-embedding support between the two build systems (just the library on Autotools, library + executables on CMake) would be a problem, as long as comments and/or the commit message make it clear what's going on.

Even if you want this version-info-embedding under both build systems, the best way to achieve it might be to merge a fixed version of your patch (with the different FILETYPE for executables, and a comment explaining what is going on), and then bolt on the more extensive Autotools implementation afterwards.

(In particular, I want to do dbus 1.12.0 soon, so I'd be tempted to delay the per-executable version info under Autotools to 1.13 to avoid adding complexity to the Autotools build system at this late stage.)

> I know that generated files need to be added to AC_CONFIG_FILES in
> configure.ac, but I do not know how to provide different variables to them.

I don't think AC_CONFIG_FILES is the right way to achieve that result in Autotools - it might not even be possible, and if it is, it would certainly be ugly and complicated.

We could generate per-executable version info resources at "make" time instead of at "configure" time by using sed in the Makefile.am, something like this:

nodist_dbus_launch_SOURCES += dbus-launch-versioninfo.rc
nodist_dbus_run_session_SOURCES += dbus-run-session-versioninfo.rc
# etc.

%-versioninfo.rc: $(top_srcdir)/dbus/versioninfo.rc.in Makefile
	sed \
		-e 's![@]INTERNAL_NAME[@]!$*!g' \
		-e 's![@]ORIGINAL_FILENAME[@]!$*$(EXEEXT)!g' \
		-e 's![@]DBUS_VERSION[@]!@DBUS_VERSION@!g' \
		-e 's![@]BUILD_TIMESTAMP[@]!@BUILD_TIMESTAMP@!g' \
		-e 's![@]FILETYPE[@]!VFT_EXECUTABLE!g' \
		... etc. ... \
		< $< > $@

Actually, I've often been tempted to do more of our current file-generating like that, and less of it with AC_CONFIG_FILES - it makes their interdependencies more visible to the build system. But that's definitely a job for 1.13.x.
Comment 96 Simon McVittie 2017-10-20 17:09:38 UTC
(In reply to Ralf Habacker from comment #93)
> Add version info to installed executables for cmake build system on Windows

Would you mind taking this additional feature to a separate bug number? This one is getting a little confusing - the scope has grown quite a lot (partly my fault, I know).

If I understand correctly, the original scope of this bug, "versioninfo.o is not added to target libdbus-1 on Windows", is fixed in git master. Is that correct?

I think at this point I'd like to save any more features/enhancements in this direction for after I've released dbus 1.12.0 (or at least branched dbus-1.12 so I can do subsequent 1.11.x and 1.12.x releases from that branch). I'd be happy to start a dbus-1.12 branch from current master any time, if you want to keep doing feature development on master. Is that OK?
Comment 97 Ralf Habacker 2017-10-21 10:04:22 UTC
moved remaining patch to bug 103387
Comment 98 Ralf Habacker 2017-10-26 21:39:40 UTC
(In reply to Simon McVittie from comment #95)
> > automake probably can have same support for executable ?
> 
> It could, but I'm not sure how worth the effort it would be. How important
> is this version info for executables? There's a limit to how important it
> can be if we've had an official Windows port for 10 years without needing
> it? :-)
Recently an msvc build of DBus on Windows has been added to a commercial app on Windows (http://www.gausz.de) to replace the former DDE based remote control.
The installation of this app provides version info of all shared libraries and executable. With msvc version info for the library was available since 2009. On looking at the recent issue we recognized that those informations were missing too.
  
> Even if you want this version-info-embedding under both build systems, the
> best way to achieve it might be to merge a fixed version of your patch (with
> the different FILETYPE for executables, and a comment explaining what is
> going on),

sure, see bug 103387

> and then bolt on the more extensive Autotools implementation afterwards.
this makes sense because autotools is used on opensuse obs to build dbus, which is included in the stable windows releases for kmymoney and umbrello.

> (In particular, I want to do dbus 1.12.0 soon, so I'd be tempted to delay
> the per-executable version info under Autotools to 1.13 to avoid adding
> complexity to the Autotools build system at this late stage.)
agreed


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.