Bug 102558

Comment on attachment 133990 [details] [review]
Patch

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

::: cmake/modules/Win32.Manifest.in
@@ +2,5 @@
> +<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
> +<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
> +<security>
> +<requestedPrivileges>
> +<requestedExecutionLevel level="asInvoker" uiAccess="true"/>

Why are we shipping a manifest source file that requests UI access if we are just going to override it to false every time we use it?

::: cmake/modules/Win32Macros.cmake
@@ +47,5 @@
> +        file(READ ${CMAKE_SOURCE_DIR}/modules/Win32.Manifest.in _tmp)
> +        string(REPLACE "true" "false" _out ${_tmp})
> +        file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/${_source}.manifest ${_out})
> +        # create rc file
> +        # see https://stackoverflow.com/questions/33000158/embed-manifest-file-to-require-administrator-execution-level-with-mingw32

Can we use symbolic constants ID_MANIFEST and RT_MANIFEST instead of the magic numbers? Some googling suggests that they might be defined if you #include <winuser.h> in the .rc file.

If we can't (for instance because they are not sufficiently portable to old Windows or old toolchains), please add comments something like this to the macro:

# 1 is the resource ID, ID_MANIFEST
# 24 is the resource type, RT_MANIFEST

::: cmake/tools/CMakeLists.txt
@@ +63,5 @@
>  install(TARGETS dbus-test-tool ${INSTALL_TARGETS_DEFAULT_ARGS})
>  
> +if(WIN32)
> +    add_win32_manifest(dbus_update_activation_environment_SOURCES)
> +endif()

We'll presumably need to behave similarly when building with Autotools. dbus/Makefile.am seems to have some code to compile a .rc file to a .o file with $(WINDRES).

Created attachment 133998 [details] [review]
cmake part of patch (updated)

Created attachment 133999 [details]
patch (autotools part)

Got autotools configure issue although I tries several variants of a configure line  (/usr/i686-w64-mingw32/sys-root/mingw/ is cross install root)

EXPAT_CFLAGS= EXPAT_LIBS=/usr/i686-w64-mingw32/sys-root/mingw/lib/libexpat.dll.a ../dbus-1/configure --host=i686-w64-mingw32

EXPAT_CFLAGS= EXPAT_LIBS="-L/usr/i686-w64-mingw32/sys-root/mingw/lib -lexpat" ../dbus-1/configure --host=i686-w64-mingw32

PKG_CONFIG_PATH=/usr/i686-w64-mingw32/sys-root/mingw/lib/pkgconfig/ ../dbus-1/configure --host=i686-w64-mingw32

Got on configuring 
checking abstract socket namespace... no
configure: WARNING: Cannot check for abstract sockets when cross-compiling, please use --enable-abstract-sockets
checking for EXPAT... configure: error: Package requirements (expat) were not met:

No package 'expat' found

Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.

Alternatively, you may set the environment variables EXPAT_CFLAGS
and EXPAT_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.

Created attachment 134079 [details] [review]
autotools part (update)

(In reply to Ralf Habacker from comment #3)
> Got autotools configure issue 
see bug 102613

Comment on attachment 133998 [details] [review]
cmake part of patch (updated)

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

::: cmake/modules/Win32.Manifest.in
@@ +6,5 @@
> +<requestedExecutionLevel level="asInvoker" uiAccess="false"/>
> +</requestedPrivileges>
> +</security>
> +</trustInfo>
> +</assembly>

Nitpick: Any chance this XML could have some indentation added to make it more readable?

Comment on attachment 134079 [details] [review]
autotools part (update)

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

::: tools/Makefile.am
@@ +119,5 @@
> +
> +.rc.o:
> +	$(WINDRES) $< -o $@
> +
> +dbus_update_activation_environment_SOURCES += $(top_builddir)/tools/duae_manifest.rc $(NULL)

The $(NULL) is not necessary here. It’s only used (by convention) as a terminator for a multi-line list to avoid introducing diff noise with the final backslash.

@@ +122,5 @@
> +
> +dbus_update_activation_environment_SOURCES += $(top_builddir)/tools/duae_manifest.rc $(NULL)
> +
> +$(top_builddir)/tools/duae_manifest.rc: $(top_srcdir)/cmake/modules/Win32.Manifest.in
> +	echo -e "1 24 \"$(top_srcdir)/cmake/modules/Win32.Manifest.in\"" > $(top_builddir)/tools/duae_manifest.rc

Use `$@` for the duae_manifest.rc target filename. Use $^ for the Win32.Manifest.in prerequisite filename.

Created attachment 134102 [details] [review]
autotools patch (update 2)

- renamed duae_manifest.rc to duae-manifest.rc
- add static library as dependency

According to https://stackoverflow.com/questions/21098033/makefile-am-how-to-add-rule-for-new-extension I tried first a similar approach by adding due-manifest.rc to dbus_update_activation_environment_SOURCES (previous patch) but this does not work. In the resulting Makefile duae-manifest.o is not added to the object list of target dbus_update_activation_environment (am__objects_1 is added to the target object list but is empty) for unknown reasons.

(In reply to Ralf Habacker from comment #8)
> - add static library as dependency
need to correct: this seems not a real static library more similar to the  cmake command "add_custom_command" (https://cmake.org/cmake/help/v3.2/command/add_custom_command.html)

> https://stackoverflow.com/questions/21098033/makefile-am-how-to-add-rule-for-
> new-extension I tried first a similar approach
which is what I used in the cmake part

(In reply to Ralf Habacker from comment #8)
> Created attachment 134102 [details] [review] [review]
> autotools patch (update 2)
> 
> - renamed duae_manifest.rc to duae-manifest.rc
> - add static library as dependency
> 
> According to
> https://stackoverflow.com/questions/21098033/makefile-am-how-to-add-rule-for-
> new-extension I tried first a similar approach by adding due-manifest.rc to
> dbus_update_activation_environment_SOURCES (previous patch) but this does
> not work. In the resulting Makefile duae-manifest.o is not added to the
> object list of target dbus_update_activation_environment (am__objects_1 is
> added to the target object list but is empty) for unknown reasons.

To be fair: it worked in a preconfigured dbus autotools based build dir which let me think adding simply the rc file to the list of target sources would work.
Unfortunally this approach did not work anymore with a freshly created build dir which forced me to use the recent approach in update 2.

Created attachment 134443 [details] [review]
Add Windows manifest to dbus-update-activation-environment.exe

From: Ralf Habacker <ralf.habacker@freenet.de>

This explicitly sets the execution level to 'asInvoker', preventing
Windows' UAC heuristics from deciding that because its name mentions
"update", it probably needs to escalate privileges.

[smcv: flatten CMake and Autotools parts into one commit, add
more explanation to commit message]

---

I don't think this makes sense as two separate patches, so I flattened them into one before reviewing properly, and took the opportunity to improve the commit message.

Comment on attachment 134443 [details] [review]
Add Windows manifest to dbus-update-activation-environment.exe

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

::: cmake/modules/Win32.Manifest.in
@@ +3,5 @@
> + <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
> +  <security>
> +   <requestedPrivileges>
> +     <requestedExecutionLevel level="asInvoker" uiAccess="false"/>
> +   </requestedPrivileges>

It seems weird that this is in cmake/modules when it has nothing in particular to do with CMake. Putting it in tools/ would seem more appropriate?

::: cmake/modules/Win32Macros.cmake
@@ +54,5 @@
> +    #   set (SOURCE test.c)
> +    #   set_execution_level_as_invoker(SOURCE)
> +    #   add_executable(atarget, ${SOURCE})
> +    #
> +    MACRO (set_execution_level_as_invoker _source)

I can't help wondering whether this macro is premature generalization - we only use this in one place! It might be better to just open-code it, like you did for Autotools.

Created attachment 134487 [details] [review]
Avoid dbus-update-activation-environment being catched by UAC

Created attachment 134488 [details] [review]
Avoid dbus-update-activation-environment being catched by UAC

(In reply to Ralf Habacker from comment #14)
> Avoid dbus-update-activation-environment being catched by UAC

Please extend the commit message to describe what's being done and why, something like Comment #11.

The English word is "caught" not "catched", but in any case I think this deserves more explanation than a one-liner.

Created attachment 134489 [details] [review]
Add Windows manifest to dbus-update-activation-environment.exe

This explicitly sets the execution level to 'asInvoker', preventing
Windows' UAC heuristics from deciding that because its name mentions
"update", it probably needs to escalate privileges.

Comment on attachment 134488 [details] [review]
Avoid dbus-update-activation-environment being catched by UAC

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

::: cmake/tools/CMakeLists.txt
@@ +62,5 @@
>  target_link_libraries(dbus-test-tool ${DBUS_LIBRARIES})
>  install(TARGETS dbus-test-tool ${INSTALL_TARGETS_DEFAULT_ARGS})
>  
> +if(WIN32)
> +    # avoid dbus_update_activation_environment being catched by uac

"avoid dbus-update-activation-environment being caught by UAC"

or perhaps better

"avoid dbus-update-activation-environment triggering UAC"

@@ +67,5 @@
> +    # 1 is the resource ID, ID_MANIFEST
> +    # 24 is the resource type, RT_MANIFEST
> +    # constants are used because of a bug in windres
> +    # see https://stackoverflow.com/questions/33000158/embed-manifest-file-to-require-administrator-execution-level-with-mingw32
> +    file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/duae-manifest.rc "1 24 \"${CMAKE_SOURCE_DIR}/../tools/Win32.Manifest.in\"\n")

I think ${CMAKE_SOURCE_DIR}/../tools is the same thing as ${CMAKE_SOURCE_DIR} here? If so, please simplify

::: tools/Makefile.am
@@ +117,5 @@
> +if DBUS_WIN
> +SUFFIXES = .rc
> +
> +.rc.o:
> +	$(WINDRES) $< -o $@

You said in Comment #8 that this wasn't enough. According to Automake documentation, what you've proposed here seems to be meant to work... but you said it failed unless you used the same -Wl,foo.o workaround that we use for libdbus' version information.

Does the nicer technique work better in this commit for some reason, such that the -Wl,foo.o workaround is no longer needed?

@@ +119,5 @@
> +
> +.rc.o:
> +	$(WINDRES) $< -o $@
> +
> +dbus_update_activation_environment_SOURCES += duae_manifest.rc

This is a generated file, so it should go in nodist_dbus_update_activation_environment_SOURCES instead.

$(nodist_dbus_update_activation_environment_SOURCES) should also be added to CLEANFILES so that we delete duae_manifest.rc in "make clean". Anything that is created by "make" should be deleted by "make clean".

Perhaps it would be better named disable-uac.rc since that's what it does? Then we wouldn't need to generate a second .rc file if we want to disable UAC in some other binary later.

@@ +121,5 @@
> +	$(WINDRES) $< -o $@
> +
> +dbus_update_activation_environment_SOURCES += duae_manifest.rc
> +
> +duae_manifest.rc: $(top_srcdir)/tools/Win32.Manifest.in

I think you could probably replace $(top_srcdir)/tools/Win32.Manifest.in with just Win32.Manifest.in? (Try it and see what travis-ci says?)

The .in extension seems redundant - foo.xml.in means "we are going to preprocess this with config.status or sed or similar to get foo.xml", but it looks like we're just using this manifest directly. So the file should probably be called disable-uac.manifest or duae.manifest or something?

@@ +129,1 @@
>  EXTRA_DIST = run-with-tmp-session-bus.sh strtoll.c strtoull.c

Win32.Manifest.in (or whatever it gets renamed to) needs adding to EXTRA_DIST, otherwise a tarball from "make distcheck" won't have it, resulting in failure to build from those tarballs for Windows.

::: tools/Win32.Manifest.in
@@ +2,5 @@
> +<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
> +<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
> +<security>
> +<requestedPrivileges>
> +<requestedExecutionLevel level="asInvoker" uiAccess="false"/>

Please indent this file (as you did in Comment #8) unless there's a technical reason why you shouldn't.

(In reply to Ralf Habacker from comment #9)
> (In reply to Ralf Habacker from comment #8)
> > - add static library as dependency
> need to correct: this seems not a real static library more similar to the 
> cmake command "add_custom_command"

FYI, it's an object file (not a static library) compiled using a manually-specified make recipe (which is analogous to a CMake custom command).

Object files (.o, .obj) are individual compiled modules, typically the result of compiling a single translation unit (C or C++ source file). In this case you're compiling a resource into object code with windres, instead of compiling C into object code with a C compiler. Presumably Win32 resources are implemented as a special case of read-only data, similar to C code containing a global const variable.

Static libraries (.a) are archives (like a zip or tar file) containing multiple object files. Passing a static library to a linker is basically a shortcut for specifying every object file that went into the static library individually.

Created attachment 134491 [details] [review]
Add Windows manifest to dbus-update-activation-environment.exe

This explicitly sets the execution level to 'asInvoker', preventing
Windows' UAC heuristics from deciding that because its name mentions
"update", it probably needs to escalate privileges.

(In reply to Simon McVittie from comment #17)
> > +    # 1 is the resource ID, ID_MANIFEST
> > +    # 24 is the resource type, RT_MANIFEST
> > +    # constants are used because of a bug in windres
> > +    # see https://stackoverflow.com/questions/33000158/embed-manifest-file-to-require-administrator-execution-level-with-mingw32
> > +    file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/duae-manifest.rc "1 24 \"${CMAKE_SOURCE_DIR}/../tools/Win32.Manifest.in\"\n")
> 
> I think ${CMAKE_SOURCE_DIR}/../tools is the same thing as
> ${CMAKE_SOURCE_DIR} here? If so, please simplify
no, CMAKE_SOURCE_DIR is <source-root>/cmake and we need to get <source-root>/tools. If cmake would live in the top level dir as autotools does your comment would be true.

> ::: tools/Makefile.am
> @@ +117,5 @@
> > +if DBUS_WIN
> > +SUFFIXES = .rc
> > +
> > +.rc.o:
> > +	$(WINDRES) $< -o $@
> 
> You said in Comment #8 that this wasn't enough. According to Automake
> documentation, what you've proposed here seems to be meant to work... but
> you said it failed unless you used the same -Wl,foo.o workaround that we use
> for libdbus' version information.
> 
> Does the nicer technique work better in this commit for some reason, such
> that the -Wl,foo.o workaround is no longer needed?

I retried with an older patch revision (therefore the missing indention in the manifest file) and it seems to work now but do not ask me why, I don't know.

> 
> @@ +119,5 @@
> > +
> > +.rc.o:
> > +	$(WINDRES) $< -o $@
> > +
> > +dbus_update_activation_environment_SOURCES += duae_manifest.rc
> 
> This is a generated file, so it should go in
> nodist_dbus_update_activation_environment_SOURCES instead.
fixed
> 
> $(nodist_dbus_update_activation_environment_SOURCES) should also be added to
> CLEANFILES so that we delete duae_manifest.rc in "make clean". Anything that
> is created by "make" should be deleted by "make clean".
fixed

> 
> Perhaps it would be better named disable-uac.rc since that's what it does?
> Then we wouldn't need to generate a second .rc file if we want to disable
> UAC in some other binary later.
fixed

> 
> @@ +121,5 @@
> > +	$(WINDRES) $< -o $@
> > +
> > +dbus_update_activation_environment_SOURCES += duae_manifest.rc
> > +
> > +duae_manifest.rc: $(top_srcdir)/tools/Win32.Manifest.in
> 
> I think you could probably replace $(top_srcdir)/tools/Win32.Manifest.in
> with just Win32.Manifest.in? (Try it and see what travis-ci says?)

not fixed yet.

> The .in extension seems redundant - foo.xml.in means "we are going to
> preprocess this with config.status or sed or similar to get foo.xml", but it
> looks like we're just using this manifest directly. So the file should
> probably be called disable-uac.manifest or duae.manifest or something?

fixed

> 
> @@ +129,1 @@
> >  EXTRA_DIST = run-with-tmp-session-bus.sh strtoll.c strtoull.c
> 
> Win32.Manifest.in (or whatever it gets renamed to) needs adding to
> EXTRA_DIST, otherwise a tarball from "make distcheck" won't have it,
> resulting in failure to build from those tarballs for Windows.
fixed

> ::: tools/Win32.Manifest.in
> @@ +2,5 @@
> > +<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
> > +<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
> > +<security>
> > +<requestedPrivileges>
> > +<requestedExecutionLevel level="asInvoker" uiAccess="false"/>

> Please indent this file (as you did in Comment #8) unless there's a
> technical reason why you shouldn't.

fixed

Created attachment 134492 [details] [review]
Add Windows manifest to dbus-update-activation-environment.exe

This explicitly sets the execution level to 'asInvoker', preventing
Windows' UAC heuristics from deciding that because its name mentions
"update", it probably needs to escalate privileges.

- remove path from reference to Win32.Manifest - it should work without

(In reply to Ralf Habacker from comment #20)
> (In reply to Simon McVittie from comment #17)
> > I think ${CMAKE_SOURCE_DIR}/../tools is the same thing as
> > ${CMAKE_SOURCE_DIR} here? If so, please simplify
>
> no, CMAKE_SOURCE_DIR is <source-root>/cmake and we need to get
> <source-root>/tools. If cmake would live in the top level dir as autotools
> does your comment would be true.

Ah, OK, CMAKE_SOURCE_DIR is more like Autotools (abs_)top_srcdir than Autotools (abs_)srcdir. Fine.

Comment on attachment 134492 [details] [review]
Add Windows manifest to dbus-update-activation-environment.exe

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

Looks good, assuming it works! Thanks for your patience on this.

aapplied to git master(In reply to Simon McVittie from comment #23)
> Comment on attachment 134492 [details] [review] [review]
> Add Windows manifest to dbus-update-activation-environment.exe
> 
> Review of attachment 134492 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good, assuming it works! Thanks for your patience on this.

We will see - patch applied to master.

Created attachment 134517 [details] [review]
Fix one remaining case not adding disable-uac.o to target dbus_update_activation_environment

Got the idea from http://fragglet.livejournal.com/4448.html.

Bug:

Comment on attachment 134517 [details] [review]
Fix one remaining case not adding disable-uac.o to target dbus_update_activation_environment

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

::: tools/Makefile.am
@@ +120,5 @@
>  .rc.o:
>  	$(WINDRES) $< -o $@
>  
> +%.o : %.rc
> +	$(WINDRES) $^ -o $@

I was about to say: You shouldn't need both .rc.o (an "implicit rule") and %.o : %.rc (a "pattern rule"). However, the Livejournal post you linked says the implicit rule is needed for Automake. I can't help thinking that indicates a bug somewhere.

I think this deserves a comment (or a note in the commit message, or both) summarizing the main point from the LJ post you linked, in case the LJ disappears: 

# This looks redundant, but it isn't. The implicit rule (.rc.o)
# is read by Automake but ignored by GNU make. The pattern rule
# (%.o: %.rc) is read by GNU make but ignored by Automake.

However, if we can, I'd prefer to fix (or at least identify) whatever bug is causing us to need this weird redundancy.

Whichever route you take here, the same techniques should be usable to fix Bug #103015. Fundamentally, this and Bug #103015 are the same bug - "Automake rules to link in Win32 resources look good, but don't actually work" - so I think it would be fine to fix them as a single commit.

(In reply to Ralf Habacker from comment #25)
> Fix one remaining case not adding disable-uac.o to target
> dbus_update_activation_environment

Is this a failure mode that you have actually observed in real life, or only theoretical? It seems to be working fine for me:

.../mingw % touch tools/disable-uac.rc
.../mingw % make V=1
...
make[2]: Entering directory '/srv/tmp/smcv/build/dbus/mingw/tools'
i686-w64-mingw32-windres disable-uac.rc -o disable-uac.o
/bin/bash ../libtool  --tag=CC   --mode=link i686-w64-mingw32-gcc -O0 -g -fprofile-arcs -ftest-coverage  -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 -Werror -Wno-suggest-attribute=format -Wno-error=unused-parameter -Wno-error=missing-field-initializers -static-libgcc -g -O0 -export-dynamic -Wl,--no-as-needed -Wl,--fatal-warnings -static-libgcc -o dbus-update-activation-environment.exe dbus-update-activation-environment.o tool-common.o disable-uac.o ../dbus/libdbus-1.la

(In reply to Simon McVittie from comment #26)
> Fundamentally, this and Bug #103015 are the same bug -
> "Automake rules to link in Win32 resources look good, but don't actually
> work"

... or not. Bug #103015 seems to have had an unrelated root cause, and it isn't clear to me whether there is any remaining bug here or not.

(In reply to Simon McVittie from comment #27)
> Is this a failure mode that you have actually observed in real life, or only
> theoretical?

If this is a failure mode that you have actually observed, please attach a build log for git master with no additional changes.

If this is only theoretical, then I think we can close this bug again, and use Bug #103015 to represent further cleanup.

Created attachment 134661 [details]
configure.log

(In reply to Simon McVittie from comment #29)
> If this is a failure mode that you have actually observed
yes
> please attach a build log for git master with no additional changes.
see appended files

libtool skips disable-uac.o in the resulting link line

Created attachment 134662 [details]
make.log

(In reply to Ralf Habacker from comment #30)
> (In reply to Simon McVittie from comment #29)
> > please attach a build log for git master with no additional changes.
> see appended files
> 
> libtool skips disable-uac.o in the resulting link line

Line 784

/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 -O2 -g -pipe -Wall -fexceptions --param=ssp-buffer-size=4 -mms-bitfields -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 

Line 788 (they are not consecutive, probably because you're doing a parallel build)

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 -O2 -g -pipe -Wall -fexceptions --param=ssp-buffer-size=4 -mms-bitfields -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

That looks like disable-uac.o is correctly present.

tools/.libs/dbus-update-activation-environment.exe is the real executable: it'll be installed if you run "make install".

tools/dbus-update-activation-environment.exe is just a libtool wrapper: on Unix it would be a shell script, but Windows doesn't have scripts with the same extension as a real executable, so it has to be a simple executable. I don't think you can avoid that one triggering UAC.

(In reply to Ralf Habacker from comment #25)
> Fix one remaining case not adding disable-uac.o to target
> dbus_update_activation_environment
> 
> Got the idea from http://fragglet.livejournal.com/4448.html.

This shouldn't be necessary, and as far as I can see, isn't necessary.

Comment on attachment 134661 [details]
configure.log

just a few notes:
- with recent patches it looks to work also on two local installations
- I did run ./autogen.sh before configuring 
- the manifest is added as clear text to the binary - I verified that it is included in the executable by loading it into an text editor

(In reply to Ralf Habacker from comment #34)
> just a few notes:
> - with recent patches it looks to work also on two local installations

Sorry, I'm confused. Does it work or not?

If it works, please close this as RESOLVED FIXED.

If it doesn't work, please provide a build log or steps to reproduce for a situation where it doesn't work.

(In reply to Simon McVittie from comment #35)
> (In reply to Ralf Habacker from comment #34)
> > just a few notes:
> > - with recent patches it looks to work also on two local installations
> 
> Sorry, I'm confused. Does it work or not?
> 
> If it works, please close this as RESOLVED FIXED.

I added the logs for the record to document how the working case should look.
Thanks for your support.

Created attachment 134898 [details] [review]
Do not add custom UAC related manifest to cmake builds for MSVC on Windows

A related manifest is added by default for msvc versions >= 8.0 (see
https://github.com/Kitware/CMake/blob/master/Modules/Platform/Windows-MSVC.cmake#L270
 for details).

Comment on attachment 134898 [details] [review]
Do not add custom UAC related manifest to cmake builds for MSVC on Windows

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

I'm not sure how CMake would know what it should put in that manifest?

If I'm reading https://github.com/Kitware/CMake/commit/e134e53b47fc9f0337529ce2b6851cec6319a8af correctly, for MSVC >= 8 we'd want to add ${CMAKE_SOURCE_DIR}/../tools/Win32.Manifest to the SOURCES instead of disable-uac.rc, and possibly rename it from .Manifest to .manifest. Is that correct? If so, please do that.

How old is MSVC 8? Are older versions something you aim to support? (My suggestion would be to pick one of the versions that Microsoft made available at no cost, and say that's our minimum; but perhaps you have reasons to do something different.)

(In reply to Simon McVittie from comment #38)
> Comment on attachment 134898 [details] [review] [review]
> Do not add custom UAC related manifest to cmake builds for MSVC on Windows
> 
> Review of attachment 134898 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure how CMake would know what it should put in that manifest?
> 
> If I'm reading
> https://github.com/Kitware/CMake/commit/
> e134e53b47fc9f0337529ce2b6851cec6319a8af correctly, for MSVC >= 8 we'd want
> to add ${CMAKE_SOURCE_DIR}/../tools/Win32.Manifest to the SOURCES instead of
> disable-uac.rc, and possibly rename it from .Manifest to .manifest. Is that
> correct? 
no,  msvc or cmake configured for msvc by default creates a manifest file with the same content as Win32.Manifest and link it into the library. A related run returns: 

C:\cegit\gausz\dev-utils\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\dbus-update-activation-environment.dir --manifests  -- C:\msvs100\VC\bin\link.exe /nologo @CMakeFiles\dbus-update-activation-environment.dir\objects1.rsp @C:\Users\admin\AppData\Local\Temp\nmBDE6.tmp
Visual Studio Incremental Link with embedded manifests
Create CMakeFiles\dbus-update-activation-environment.dir/manifest.rc
RC Pass 1:
C:/winsdk-v7.0A/bin/RC.Exe /foCMakeFiles\dbus-update-activation-environment.dir/manifest.res CMakeFiles\dbus-update-activation-environment.dir/manifest.rc
LINK Pass 1:
C:\msvs100\VC\bin\link.exe /nologo @CMakeFiles\dbus-update-activation-environment.dir\objects1.rsp /out:..\bin\dbus-update-activation-environment.exe /implib:..\bin\dbus-update-activation-environment.lib /pdb:C:\cegit\gausz\build\cegit\dbus-src-all\work\msvc2010-Debug-gitHEAD-wh\bin\dbus-update-activation-environment.pdb /version:0.0 /machine:X86 /debug /INCREMENTAL /subsystem:console -LIBPATH:C:\cegit\gausz\build\cegit\dbus-src-all\work\msvc2010-Debug-gitHEAD-wh\bin ..\bin\dbus-1.lib ws2_32.lib advapi32.lib netapi32.lib iphlpapi.lib dbghelp.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:CMakeFiles\dbus-update-activation-environment.dir/intermediate.manifest CMakeFiles\dbus-update-activation-environment.dir/manifest.res
LINK : ..\bin\dbus-update-activation-environment.exe wurde nicht gefunden oder beim letzten inkrementellen Linkvorgang nicht erstellt; vollstõndiger Link wird durchgef³hrt.
MT:
C:/winsdk-v7.0A/bin/mt.exe /nologo /manifest CMakeFiles\dbus-update-activation-environment.dir/intermediate.manifest /out:CMakeFiles\dbus-update-activation-environment.dir/embed.manifest /notify_update

> 
> How old is MSVC 8? Are older versions something you aim to support? (My
> suggestion would be to pick one of the versions that Microsoft made
> available at no cost, and say that's our minimum; but perhaps you have
> reasons to do something different.)
I need to think about this

(In reply to Ralf Habacker from comment #39)
> (In reply to Simon McVittie from comment #38)
> > I'm not sure how CMake would know what it should put in that manifest?
>
>  msvc or cmake configured for msvc by default creates a manifest file
> with the same content as Win32.Manifest and link it into the library.

Huh. That isn't what I'd expect, but if it works...

How old is the oldest version of MSVC that does this?

(In reply to Simon McVittie from comment #40)
> (In reply to Ralf Habacker from comment #39)
> > (In reply to Simon McVittie from comment #38)
> > > I'm not sure how CMake would know what it should put in that manifest?
> >
> >  msvc or cmake configured for msvc by default creates a manifest file
> > with the same content as Win32.Manifest and link it into the library.
> 
> Huh. That isn't what I'd expect, but if it works...

I tried that with msvc 10.0 from 2010. 

> How old is the oldest version of MSVC that does this?

according to cmake is that MSVC++ 8.0 ( _MSC_VER == 1400), which is Visual Studio 2005 released at 2005.

(In reply to Ralf Habacker from comment #41)
> > Huh. That isn't what I'd expect, but if it works...
> 
> I tried that with msvc 10.0 from 2010. 
> 
> > How old is the oldest version of MSVC that does this?
> 
> according to cmake is that MSVC++ 8.0 ( _MSC_VER == 1400), which is Visual
> Studio 2005 released at 2005.

I don't think we need to try to support pre-2005 compilers, so if this works better for you with MSVC, go ahead.

Please amend the commit message to be a bit clearer that the manifest is really equivalent (if it is, as you said in Comment #39) - perhaps replace "A related manifest" with "An equivalent manifest"? If you can find some official documentation of this feature on MSDN or similar, a link to that would also be great.

Created attachment 134925 [details] [review]
Do not add custom UAC related manifest to cmake builds for MSVC on Windows

MSVC compiler >= 8.0 (VS 2005) add an identical manifest (with uac level
set to 'asInvoker' specified by /MANIFEST) by default to generated binaries
(see https://msdn.microsoft.com/en-us/library/f2c0w594.aspx for details).

Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Bug:

Comment on attachment 134925 [details] [review]
Do not add custom UAC related manifest to cmake builds for MSVC on Windows

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

Thanks, looks good. The reference you've provided here makes it a lot more clearly correct.

(In reply to Simon McVittie from comment #44)
> Comment on attachment 134925 [details] [review] [review]
> Do not add custom UAC related manifest to cmake builds for MSVC on Windows
> 
> Review of attachment 134925 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks, looks good. The reference you've provided here makes it a lot more
> clearly correct.

Just a note: this patch requires attachment 134924 [details] [review]

(In reply to Ralf Habacker from comment #45)
> > Do not add custom UAC related manifest to cmake builds for MSVC on Windows
> 
> Just a note: this patch requires attachment 134924 [details] [review]

... from Bug #103015.

What is the impact of not applying this patch? Does its absence break the build, or is it just simplification?

I ask because I want to do a dbus-1.12.0 release sometime soon, and I need to decide what we can get in before 1.12.x and what would be safer to defer to 1.13.x.

(In reply to Simon McVittie from comment #46)
> (In reply to Ralf Habacker from comment #45)
> > > Do not add custom UAC related manifest to cmake builds for MSVC on Windows
> > 
> > Just a note: this patch requires attachment 134924 [details] [review] [review]
> apply 
> ... from Bug #103015.
> 
> What is the impact of not applying this patch? Does its absence break the
> build, 
yes, msvc fails to link with a "duplicate resource file type" 

> or is it just simplification?
> 
> I ask because I want to do a dbus-1.12.0 release sometime soon, and I need
> to decide what we can get in before 1.12.x and what would be safer to defer
> to 1.13.x.
If the cmake patch for versionsinfo on executables from Bug #103015 will be applied, the remaining patch from this bug is mandatory - either none or both should be applied

(In reply to Ralf Habacker from comment #47)
> yes, msvc fails to link with a "duplicate resource file type" 

We should stop discussing this on a closed bug: I keep losing track of it :-(

I think you have already answered this question, but to be completely clear about it: is this failure present in current master (6ad07c4c), or does it only appear after attaching the "version info in executables" patch that you attached to #103015?

Option 1: If current master fails to build with MSVC, then I think we should fix that before 1.12.0: please reopen this bug or open a new one, and attach a patch that does not depend on the one from #103015.

Option 2: If current master builds successfully with MSVC, but the "version info in executables" patch breaks the build, then we should fix that as part of the the "version info in executables" feature (which is currently in #103015, but I think it should probably become a separate enhancement-level bug number); and at this point, I would prefer to do that in 1.13.0, unless the feature is really important for some reason that I'm not seeing.

> If the cmake patch for versionsinfo on executables from Bug #103015 will be
> applied, the remaining patch from this bug is mandatory - either none or
> both should be applied

If I'm understanding this correctly, then we are in what I called "Option 2" above.

Created attachment 134968 [details] [review]
Do not add custom UAC related manifest to cmake builds for MSVC on Windows

MSVC compiler >= 8.0 (VS 2005) add an identical manifest (with uac level
set to 'asInvoker' specified by /MANIFEST) by default to generated binaries
(see https://msdn.microsoft.com/en-us/library/f2c0w594.aspx for details).

- do not depend on patches from bug 103015

(In reply to Simon McVittie from comment #48)
> Option 1: If current master fails to build with MSVC, then I think we should
> fix that before 1.12.0: please reopen this bug or open a new one, and attach
> a patch that does not depend on the one from #103015.
this is the recent case - see appended patch

Comment on attachment 134968 [details] [review]
Do not add custom UAC related manifest to cmake builds for MSVC on Windows

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

Looks good. I'll make sure to apply this before releasing 1.11.24 (1.12.0rc1), which I'm hopefully going to do today.

(In reply to Simon McVittie from comment #51)
> before releasing 1.11.24 (1.12.0rc1)

Er, that should say 1.11.22. I'm losing track of versions :-(

Patch applied for 1.11.22, hopefully fixing the MSVC build regression.