Re: Repost: [PATCH] [cygwin|mingw] Create UAC manifest files.

[Charles Wilson]

Ralf Wildenhues wrote:
> * Charles Wilson wrote on Sun, Feb 21, 2010 at 07:31:15AM CET:
>>      [cygwin|mingw] Create UAC manifest files.
>>
>>      * libltdl/config/ltmain.m4sh (func_emit_exe_manifest): New
>>      function.
>>      (func_mode_link) [cygwin|mingw]: Create manifest files for
>>      wrapper and target exe when target name matches heuristic that
>>      triggers UAC problems for newer win32 OSs. Clean up
>>      $cwrapper.manifest on error. Ensure manifest files have
>>      executable permission.
>>      (func_mode_uninstall): Clean up manifest files.
>>      Various reports by Eric Blake, Kai Tietz, and Cesar Strauss.
> 
> I have the following issues/questions with this patch:
> 
> 0) I still haven't fully understood the semantics here.  Are the
> manifest files only needed at link editor time or at runtime? 

runtime.

> If the
> latter, then I don't see how the manifest file ever gets to be installed
> at `make install' time, nor uninstalled at `make uninstall' time?

It was a deliberate choice -- a shortcut -- to NOT install the manifest.
 The RTTD for packages which are intended to build executables with
"bad" names is to handle the manifest stuff themselves.

Well, the REAL RTTD is for automake to do it, in the non-libtool case,
but...that gets very sticky, very quickly.  And it really doesn't
address the issue with libtool-built exes (and wrapper exes).

This implementation is intended as a convenience so that the /build/
doesn't die for hard-to-explain reasons, on Vista and above.  Without
it, the build dies because we try to use wrapper.exe --dump-script to
generate the wrapper script, and that fails on Vista when the
wrapper.exe has a "bad" name and no .manifest.  We chose to use
--dump-script, and not func_emit_wrapper(), because this way allows us
to detect, and fail, if the wrapper exe doesn't build correctly for some
reason.  Arguably, not having a .manifest when you need one IS a failure
-- because if ${builddir}/wrapper.exe fails to execute due to the lack
of one, then ${builddir}/.libs/actual.exe won't have a .manifest either,
and IT will fail, as well.  Detect errors early, and all that...


I'm ignoring entirely the compiled-in .manifest resource issue.  That's
just too ugly for words (below).


Now, this interacts with an issue below, but I'll hint at it here: even
if the source provides an explicit manifest file, it's over in the
${srcdir}, and not necessarily in the ${builddir}.  But, to be
effective, the .manifest file must be in the same directory as the
identically-named .exe.  This leaves two open issues:
a) what to do when during mode=build, when ${builddir}==${srcdir}, AND
the upstream source provides an explicit .manifest file
b) what to do during 'make clean' and/or libtool --mode=clean.

> If the former, then why doesn't a --mode=link remove the manifest file
> before exiting (non-interrupted)?

n/a

> 1) --mode=clean removes .manifest files even if --mode=link did not
> create them, namely when the name of the program does not match
> *instal*|*patch*|*setup*|*update*.

Hmm.  How about if the generated manifest file includes a comment, that
specifies it was automatically generated by libtool, and then
--mode=clean only removes it from the ${builddir} if the file (a)
exists, and (b) contains that comment?  (It would add .libs/*.manifest
to $rmfiles only after an explicit existence check, but no corresponding
'have libtool comment' check...)

And, we only create a ${builddir}/[.libs/]foo.exe.manifest if there
isn't one in ${srcdir}.  If there IS, then we just copy it over to both
${builddir}/ and ${builddir}/.libs/.

For executables with "good" names, we never generate a .manifest, but if
${builddir}!=${srcdir} AND there is a .manifest in ${srcdir}, then we
copy it over to both ${builddir}/ and ${builddir}/.libs.

> Neither this nor manifest creation
> and usage, nor (0) are covered in the libtool testsuite.  But need to
> be.

So...deliberately create an exe with a "bad" name, and make sure it's
executable (plus an explicit "hey does the .manifest file exist" check)?

For the no-.manifest in srcdir, and with-.manifest in srcdir cases?

Then, for an exe with a "good" name, verify that no .manifest is
created, unless there already is one in ${srcdir}, and in that case,
that the .manifest does NOT have the "magic" comment string.

So, a total of four tests.

Would that be sufficient?

> 2) automake/lib/am/progs.am does not use --mode=clean for cleaning
> programs which means the manifest files for cwrappers will be leftover.
> I expect this to cause a distcheck failure.  

Ugh. You're right.

> So Automake needs to be
> fixed.

Well, switching to libtool --mode=clean is a big change, even if only
for the libtool-clean: rule.  Maybe a less disruptive change to progs.am:

untested pseudocode addition:

 ?LIBTOOL?       test -n "$(EXEEXT)" || exit 0; \
 ?LIBTOOL?       list=`for p in $$list; do echo "$$p"; done | ...
 ?LIBTOOL?       echo " rm -f" $$list; \
 ?LIBTOOL?       rm -f $$list
+?LIBTOOL?  for p in $$list; do
+?LIBTOOL?    if [ -e ${p}${EXEEXT}.manifest -a \
+?LIBTOOL?      grep "magic" ${p}${EXEEXT}.manifest >/dev/null 2>&1 ];
+?LIBTOOL?    then
+?LIBTOOL?      rm -f ${p}${EXEEXT}.manifest
+?LIBTOOL?    fi
+?LIBTOOL?  done

>  I'm willing to change Automake to use 'libtool --mode=clean' for
> when new-enough libtool is used, but the comment
> 
> ## FIXME: In the future (i.e., when it works) it would be nice to delegate
> ## this task to `libtool --mode=clean'.
> 
> still requires some history digging for cases where this doesn't work as
> expected.  These cases should be investigated and covered by the libtool
> testsuite.

Yeah, but that's orthogonal to this patch.  I'd rather use a stop-gap in
Automake, as above, and delay a wholesale switch to 'libtool
--mode=clean', because THAT should be investigated and dealt with as the
major issue/change that it is, and not tacked on to this minor tweak.

> 3) This patch will overwrite a .manifest file created in the build dir,
> thus may break a package that create them through some other means.

Yeah, maybe using the same 'magic' comment technique, we can generate
${builddir}*.manifest (a) if one does not already exist, and (b) there
isn't one in ${srcdir}, or (c) if one DOES already exist, but contains
the magic comment (so would be safe to over-write).

> Note that here, I'm not speaking about the manifest below .libs, and I
> understand that that is necessary as well, and that if the user creates
> one, it is probably a bug that we don't use that for the to-be-installed
> program; not sure if it's always harmless to then-reuse that for the
> cwrapper as well.
> 
> IIUC then this particular regression could be worked around inside a
> package by using an embedded manifest, since that will override an
> external .manifest file.  Right?

Yes.

> Of course libtool shouldn't remove a user-created manifest file as in
> (1) at all.  Not sure how to fix that if (0) is "runtime".

See magic comment technique, above.

> 4) How many code instances does this patch help *in practice*?  I
> suppose you guys know since you've used this for a lot of packages,
> but couldn't find data in the cited email threads.  I'd like to gauge
> the importance of this being fixed in libtool against the regressions
> this patch may cause, and the cost of waiting (or not waiting) for an
> Automake fix to propagate.

The following packages, on my cygwin installation, include .manifest
files for certain exe's:

coreutils (install)
texinfo (install-info)
patch
gtk2 (gtk-update-icon-cache)
shared-mime-info (update-mime-database)

I'm sure there are others which I don't personally have installed.  And
some of these may utilize their own methods, rather than libtool, of
creating manifest files *for installation*.  I'm pretty sure none of
them do anything manually to allow the uninstalled .exe's to operate
(such as manually creating a .libs/*.manifest file).

> The alternative to this patch would be workarounds in said packages

I think coreutils and texinfo's *cygwin* build scripts (think gentoo
emerge or debian rules) manually install an external .manifest file, but
that operation is not integrated into the automake Makefile.am/libtool
build framework.

> to
> add explicit code to create embedded manifests (that then apply to the
> to-be-installed program).  

Yes, but that's quite ugly:

#if (bad name)
# if (cygwin || mingw) && !msvc [3]
   generate .rc file [1]
   generate .manifest file [2]
   windres --input foo.rc --output foo_manifest.o --output-format=coff
   FOO_OBJECTS+=foo_manifest.o
# elif msvc
   use mt tool
# else
   do nothing
# endif
#endif

[1] foo.rc
#include "winuser.h"
1 RT_MANIFEST foo.exe.manifest

[2] foo.exe.manifest
same as current external .manifest, as generated by this patch to libtool

[3] maybe with appropriate suffix rules, these four statements could all
be replace with just "FOO_OBJECTS+=foo_manifest.o" but you'd still need
to use BUILT_SOURCES or something.

I think, maybe, that if *automake* does this, it shouldn't be automatic.
Instead, it should be an AM_INIT option, e.g. AM_INIT([win32-manifest]).
 And this win32-manifest framework tests for the existence of a manually
created 'foo.exe.manifest' (or foo.exe.manifest.in, if you use AC_OUTPUT
to populate certain fields) before actually DOING anything, and never
tries itself to autogenerate a .manifest source file.

Unlike (this proposed patch to) libtool.

> We'd still need a manifest for the cwrapper.
> Hmm, that means waiting for (2) is moot, unless we embed a manifest into
> the cwrapper.  Ugh.

Not to mention that it duplicates the 'howto make an embedded manifest'
machinery in two places: automake AND libtool.

All of these issues are why Yaakov and I punted: I don't think anybody
has enough experience yet with the whole manifest issue to be able to
definitively say exactly what should be done, and enshrine that into
automake.  So, this patch takes the bare-minimum approach: just make
sure that the *build* (and perhaps, 'make check') won't fail, by
ensuring that ./wrapper.exe --lt-dump-script continues to work, and
invoking the uninstalled ./wrapper (and thus .libs/actual.exe) during a
testsuite won't break.

Handling the case of the *installed* executable is a bigger problem, and
we just decided to, for now, leave that to external machinery (e.g.
cygport or other 'build scripts') outside of the intrinsic auto* framework.

> 5) I'd prefer a nod from Peter R. as to whether this is fixable easily:
> 
>> The only comment NOT addressed by this version is: Peter Rosin doesn't
>> want this code activated for msvc, but I'm not sure this patch should be
>> modified to guard against a compiler, whose support (in Peter's branch)
>> has not itself yet been merged into master.  And I don't know what
>> symbol to use; maybe Peter's branch has a special variable that should
>> be tested?
>>
>> Anyway, IF this patch should be modified to "protect" msvc, I'd sure
>> like somebody to tell me how that should be done:
>>
>>               # note: this script will not be executed, so do not chmod.
>>               if test "x$build" = "x$host" ; then
>> +               # Create the UAC manifests first if necessary (but the
>> +               # manifest files must have executable permission
>> regardless).
>>
>> like this, maybe:
>>              if [ $CC != msvc ]; then
>>
>> +               case $output_name in
>> +                 *instal*|*patch*|*setup*|*update*)
>>
>> Or tell me that such protection should be deferred until Peter's stuff
>> is merged?
> 
> Well, my guess is that this should be addressed at the time master is
> next merged to pr-msvc-support, either in the merge or in a followup
> commit.  To prevent even more friction caused from my incredibly slow
> progress on libtool, releases, and merging of said branch, I'd try to
> help with that merge if that helps, and would like to apologize again
> to Peter.

My hunch is that it will be an relatively easy modification to this
patch once it is "finalized", I just don't know enough about Peter's
framework to say for sure. You're right that we should wait for Peter to
chime in.

> 6)
>> +  case $host in
>> +  i?86-*-* )   echo '     processorArchitecture="x86"' ;;
>> +  ia64-*-* )   echo '     processorArchitecture="ia64"' ;;
>> +  x86_64-*-* ) echo '     processorArchitecture="amd64"' ;;
>> +  *)           echo '     processorArchitecture="*"' ;;
>> +  esac
> 
> Please use an intermediate variable to save on code size here and save
> one cat instance, i.e., move the case statement up.

Ack.

--
Chuck