|
Description
Ralf Habacker
2017-09-27 23:00:57 UTC
Created attachment 134523 [details]
log file
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. Also, we're using SUFFIXES wrong. It should contain .rc, not just rc. (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? :'-( 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. Created attachment 134535 [details] [review] dbus: Clarify why we are not just adding the resource file to SOURCES 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. 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. 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 on attachment 134534 [details] [review] dbus: Make SUFFIXES more specific Review of attachment 134534 [details] [review]: ----------------------------------------------------------------- ++ 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 on attachment 134536 [details] [review] dbus: Actually link versioninfo.o into libdbus Review of attachment 134536 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 134537 [details] [review] build: Remove unused variables Review of attachment 134537 [details] [review]: ----------------------------------------------------------------- ++ (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 ? (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 (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 on attachment 134534 [details] [review] dbus: Make SUFFIXES more specific applied to master Comment on attachment 134535 [details] [review] dbus: Clarify why we are not just adding the resource file to SOURCES applied to master Comment on attachment 134536 [details] [review] dbus: Actually link versioninfo.o into libdbus applied to master Comment on attachment 134537 [details] [review] build: Remove unused variables applied to master 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. 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. 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. (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 (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 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 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 on attachment 134546 [details] [review] Windows: Check for $RC, not $WINDRES Review of attachment 134546 [details] [review]: ----------------------------------------------------------------- ++ 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? (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 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. (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) (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. (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? 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. Sorry, I'm not going to make any further progress on this without a complete build log. Created attachment 134695 [details]
configure.log
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. (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. (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. (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. 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 :-) 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. 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 on attachment 134716 [details] [review] Update versioninfo.rc.in will be superseeded Created attachment 134719 [details] [review] - match StringFileInfo with VarFileInfo (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 ? (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 (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. (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]). (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 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 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 on attachment 134546 [details] [review] Windows: Check for $RC, not $WINDRES Review of attachment 134546 [details] [review]: ----------------------------------------------------------------- looks good (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 (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. (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 on attachment 134544 [details] [review] Windows: Simplify compiling versioninfo.rc by using libtool facilities Applied for 1.11.22, commit 7250c73e Comment on attachment 134565 [details] [review] Windows: Use libtool-detected RC to compile resources in tools/ Review of attachment 134565 [details] [review]: ----------------------------------------------------------------- looks good 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: 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 on attachment 134703 [details] [review] UNTESTED: cmake: Stop creating an empty afxres.h Review of attachment 134703 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 134704 [details] [review] UNTESTED: Windows: Stop manipulating line numbering in versioninfo.rc Review of attachment 134704 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 134703 [details] [review] UNTESTED: cmake: Stop creating an empty afxres.h tested with msvc and applied to master Comment on attachment 134704 [details] [review] UNTESTED: Windows: Stop manipulating line numbering in versioninfo.rc test with msvc and applied to master 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: 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: Created attachment 134887 [details] [review] - update commit message - skip timestamp generating patch (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). (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 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 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 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. (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. (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.) (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. 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). (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. (In reply to Ralf Habacker from comment #78) > I will update the commit message, thanks for this pointer. sorry for double posting 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 (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 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 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 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. (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" (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). 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> 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 on attachment 134910 [details] [review] Update versioninfo.rc.in Review of attachment 134910 [details] [review]: ----------------------------------------------------------------- Looks good, go ahead 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 Created attachment 134913 [details] [review] Add version info to installed executables for cmake build system on Windows Bug: 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. Created attachment 134924 [details] [review] Add version info to installed executables for cmake build system on Windows Bug: (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. (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. (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? moved remaining patch to bug 103387 (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.