bug#12993: Wrong icon for Cygw32-Emacs

bug#12993: Wrong icon for Cygw32-Emacs

[Angelo Graziosi]

I notice that the Cygw32 port does not show the standard Emacs icon on 
the upper-left corner (and elsewhere, I think).

Would it be possible to have that? The Windows port has...

Ciao,
  Angelo.

bug#12993: Wrong icon for Cygw32-Emacs

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

On 11/25/12 6:49 AM, Angelo Graziosi wrote:
> I notice that the Cygw32 port does not show the standard Emacs icon on
> the upper-left corner (and elsewhere, I think).
> 
> Would it be possible to have that? The Windows port has...

I've pushed a change to the emacs-24 branch that should resolve the
problem. We weren't compiling Windows resources into the cygw32 Emacs
binary.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Sun, 09 Dec 2012 23:13:35 -0800
> From: Daniel Colascione <dancol@dancol.org>
> Cc: 12993@debbugs.gnu.org
> 
> On 11/25/12 6:49 AM, Angelo Graziosi wrote:
> > I notice that the Cygw32 port does not show the standard Emacs icon on
> > the upper-left corner (and elsewhere, I think).
> > 
> > Would it be possible to have that? The Windows port has...
> 
> I've pushed a change to the emacs-24 branch that should resolve the
> problem. We weren't compiling Windows resources into the cygw32 Emacs
> binary.

Why are such changes committed to the release branch?  This is a minor
feature, certainly not a bugfix, while the changes are definitely
non-trivial.  Please don't do that without discussion.

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 10 Dec 2012 15:40:18 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 12993@debbugs.gnu.org, angelo.graziosi@alice.it
> 
> > I've pushed a change to the emacs-24 branch that should resolve the
> > problem. We weren't compiling Windows resources into the cygw32 Emacs
> > binary.
> 
> Why are such changes committed to the release branch?  This is a minor
> feature, certainly not a bugfix, while the changes are definitely
> non-trivial.  Please don't do that without discussion.

The more I look at the changes, the less I like them.  E.g., how do we
know that the various Windows utilities that access the resources will
DTRT with forward slashes?  Any references on the subject?

And what is this hunk about:

    === modified file 'src/unexw32.c'
    --- a/src/unexw32.c	2012-10-17 19:02:44 +0000
    +++ b/src/unexw32.c	2012-12-10 07:11:21 +0000
    @@ -85,13 +85,6 @@

     PIMAGE_SECTION_HEADER heap_section;

    -#ifdef HAVE_NTGUI
    -extern HINSTANCE hinst;
    -HINSTANCE hprevinst = NULL;
    -LPSTR lpCmdLine = "";
    -int nCmdShow = 0;
    -#endif /* HAVE_NTGUI */
    -
     /* Startup code for running on NT.  When we are running as the dumped
	version, we need to bootstrap our heap and .bss section into our
	address space before we can actually hand off control to the startup
    @@ -121,15 +114,6 @@
       /* Prevent Emacs from being locked up (eg. in batch mode) when
	  accessing devices that aren't mounted (eg. removable media drives).  */
       SetErrorMode (SEM_FAILCRITICALERRORS);
    -
    -  /* Invoke the NT CRT startup routine now that our housecleaning
    -     is finished.  */
    -#ifdef HAVE_NTGUI
    -  /* determine WinMain args like crt0.c does */
    -  hinst = GetModuleHandle (NULL);
    -  lpCmdLine = GetCommandLine ();
    -  nCmdShow = SW_SHOWDEFAULT;
    -#endif
       mainCRTStartup ();
     }

What do you know about lpCmdLine and nCmdShow, and "what crt0.c does"
with them, to be sure they can be removed?

And this hunk breaks the MS-DOS build:

    -temacs$(EXEEXT): $(START_FILES) stamp-oldxmenu $(obj) $(otherobj) $(lib)/libgnu.a
    +temacs$(EXEEXT): $(START_FILES) stamp-oldxmenu $(obj) $(otherobj) \
    +	         $(lib)/libgnu.a $(W32_RES)
	    $(CC) $(LD_FIRSTFLAG) $(ALL_CFLAGS) $(TEMACS_LDFLAGS) $(TEMACS_LDFLAGS2) \
    -	  -o temacs $(START_FILES) $(obj) $(otherobj) $(lib)/libgnu.a $(LIBES)
    +	  -o temacs $(START_FILES) $(obj) $(otherobj) $(lib)/libgnu.a $(LIBES) \
    +	  $(W32_RES_LINK)
	    test "$(CANNOT_DUMP)" = "yes" || \
	      test "X$(PAXCTL)" = X || $(PAXCTL) -r temacs$(EXEEXT)

(because W32_RES will be expanded to @W32_RES@ and W32_RES_LINK to
@W32_RES_LINK@, and GCC will barf).

Any reasons why not revert this changeset and apply it to the trunk
instead?

bug#12993: Wrong icon for Cygw32-Emacs

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On 12/10/12 5:40 AM, Eli Zaretskii wrote:>> Date: Sun, 09 Dec 2012
23:13:35 -0800
>> From: Daniel Colascione <dancol@dancol.org>
>> Cc: 12993@debbugs.gnu.org
>>
>> On 11/25/12 6:49 AM, Angelo Graziosi wrote:
>>> I notice that the Cygw32 port does not show the standard Emacs icon on
>>> the upper-left corner (and elsewhere, I think).
>>>
>>> Would it be possible to have that? The Windows port has...
>>
>> I've pushed a change to the emacs-24 branch that should resolve the
>> problem. We weren't compiling Windows resources into the cygw32 Emacs
>> binary.
>
> Why are such changes committed to the release branch?  This is a minor
> feature, certainly not a bugfix, while the changes are definitely
> non-trivial.  Please don't do that without discussion.
>

It's a minor bugfix, not a feature. I was under the impression that
the release branch was _for_ minor bugfixes. Enough people have
complained about the icon missing from the cygw32 build that I thought
it would be worthwhile to fix it before release.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

bug#12993: Wrong icon for Cygw32-Emacs

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 3533 bytes --]

On 12/10/12 6:08 AM, Eli Zaretskii wrote:
>> Date: Mon, 10 Dec 2012 15:40:18 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 12993@debbugs.gnu.org, angelo.graziosi@alice.it
>>
>>> I've pushed a change to the emacs-24 branch that should resolve the
>>> problem. We weren't compiling Windows resources into the cygw32 Emacs
>>> binary.
>>
>> Why are such changes committed to the release branch?  This is a minor
>> feature, certainly not a bugfix

It looks more like a bugfix to me. Setting the window icon is
something we should have always done, and that we didn't do so is
problem the cygw32 build had versus the nt build, with which it should
have feature parity. It's wrong and generated user complaints. That
sounds like a bug to me. I'd also mentioned on the mailing list before
that I intended to fix this problem before release.

> The more I look at the changes, the less I like them.  E.g., how do we
> know that the various Windows utilities that access the resources will
> DTRT with forward slashes?  Any references on the subject?

The resources stored in the binary don't contain paths. (I checked
with a hex editor to make sure.)

> And what is this hunk about:
> 
>     === modified file 'src/unexw32.c'
>     --- a/src/unexw32.c	2012-10-17 19:02:44 +0000
>     +++ b/src/unexw32.c	2012-12-10 07:11:21 +0000
>     @@ -85,13 +85,6 @@
> 
>      PIMAGE_SECTION_HEADER heap_section;
> 
>     -#ifdef HAVE_NTGUI
>     -extern HINSTANCE hinst;
>     -HINSTANCE hprevinst = NULL;
>     -LPSTR lpCmdLine = "";
>     -int nCmdShow = 0;
>     -#endif /* HAVE_NTGUI */
>     -
>      /* Startup code for running on NT.  When we are running as the dumped
> 	version, we need to bootstrap our heap and .bss section into our
> 	address space before we can actually hand off control to the startup
>     @@ -121,15 +114,6 @@
>        /* Prevent Emacs from being locked up (eg. in batch mode) when
> 	  accessing devices that aren't mounted (eg. removable media drives).  */
>        SetErrorMode (SEM_FAILCRITICALERRORS);
>     -
>     -  /* Invoke the NT CRT startup routine now that our housecleaning
>     -     is finished.  */
>     -#ifdef HAVE_NTGUI
>     -  /* determine WinMain args like crt0.c does */
>     -  hinst = GetModuleHandle (NULL);
>     -  lpCmdLine = GetCommandLine ();
>     -  nCmdShow = SW_SHOWDEFAULT;
>     -#endif
>        mainCRTStartup ();
>      }
> 
> What do you know about lpCmdLine and nCmdShow, and "what crt0.c does"
> with them, to be sure they can be removed?

I removed dead code. Those variables are never used; the C runtime (to
which we dynamically link anyway) handles this initialization
internally. (We appear to have a separate bug with respect to
nCmdShow: create a shortcut to runemacs.exe and set it to start
minimized or start maximized. Emacs starts with a normal window.)

The setting of hinst specifically now happens in cache_system_info, so
we don't need to do it in _start, which doesn't run for Cygwin builds.

I tested the change, and it didn't cause any functional regressions
related to command-line parsing.

> And this hunk breaks the MS-DOS build:

*sigh* The MS-DOS build breaks when the wind blows the wrong way.

Does the build work inside DOSBox? I'll have to start testing the
build in that environment before pushing.

> Any reasons why not revert this changeset and apply it to the trunk
> instead?

The missing icon annoys users. Do we really want to ship that way?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 10 Dec 2012 08:03:20 -0800
> From: Daniel Colascione <dancol@dancol.org>
> CC: angelo.graziosi@alice.it, 12993@debbugs.gnu.org
> 
> > Why are such changes committed to the release branch?  This is a minor
> > feature, certainly not a bugfix, while the changes are definitely
> > non-trivial.  Please don't do that without discussion.
> >
> 
> It's a minor bugfix, not a feature.

I guess we disagree about the definitions of these.  Me, I don't see
how not seeing a smalll icon in a build that was never supported by
Emacs until now can be a bug.

> I was under the impression that the release branch was _for_ minor
> bugfixes. Enough people have complained about the icon missing from
> the cygw32 build that I thought it would be worthwhile to fix it
> before release.

If it were a simple fix, perhaps.  But it isn't, and it touches 2
other builds, that don't have this problem, in non-trivial ways.

If you insist on having this in 24.3, then please at least make it
Cygwin-only.  For the source changes, this is easy -- use #ifdef.  For
emacs.rc, I hope you can do the same.  If not, I'd go as far as
providing a separate cygw32.rc file.

The MSDOS problems are on me -- you will owe me ;-)

OK?

bug#12993: Wrong icon for Cygw32-Emacs

[Stefan Monnier]

> It's a minor bugfix, not a feature.  I was under the impression that
> the release branch was _for_ minor bugfixes.  Enough people have
> complained about the icon missing from the cygw32 build that I thought
> it would be worthwhile to fix it before release.

The branch is in "regression only" state, i.e. only regressions w.r.t
Emacs>=23 should make it there (except for trivially safe bug-fixes, and
exceptions granted on a case-by-case basis).

Please move your patch from the branch to the trunk, since it doesn't
fix such a regression, is not super important (yes, it's definitely
nicer to have an icon, but people can live without one until 24.4), and
its safety is not obvious.


        Stefan

bug#12993: Wrong icon for Cygw32-Emacs

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On 12/10/12 9:30 AM, Stefan Monnier wrote:
>> It's a minor bugfix, not a feature.  I was under the impression that
>> the release branch was _for_ minor bugfixes.  Enough people have
>> complained about the icon missing from the cygw32 build that I thought
>> it would be worthwhile to fix it before release.
> 
> The branch is in "regression only" state, i.e. only regressions w.r.t
> Emacs>=23 should make it there (except for trivially safe bug-fixes, and
> exceptions granted on a case-by-case basis).
> 
> Please move your patch from the branch to the trunk, since it doesn't
> fix such a regression, is not super important (yes, it's definitely
> nicer to have an icon, but people can live without one until 24.4), and
> its safety is not obvious.

Done.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 10 Dec 2012 08:24:30 -0800
> From: Daniel Colascione <dancol@dancol.org>
> CC: 12993@debbugs.gnu.org, angelo.graziosi@alice.it
> 
> >        SetErrorMode (SEM_FAILCRITICALERRORS);
> >     -
> >     -  /* Invoke the NT CRT startup routine now that our housecleaning
> >     -     is finished.  */
> >     -#ifdef HAVE_NTGUI
> >     -  /* determine WinMain args like crt0.c does */
> >     -  hinst = GetModuleHandle (NULL);
> >     -  lpCmdLine = GetCommandLine ();
> >     -  nCmdShow = SW_SHOWDEFAULT;
> >     -#endif
> >        mainCRTStartup ();
> >      }
> > 
> > What do you know about lpCmdLine and nCmdShow, and "what crt0.c does"
> > with them, to be sure they can be removed?
> 
> I removed dead code. Those variables are never used; the C runtime (to
> which we dynamically link anyway) handles this initialization
> internally.

I'm not sure.  To me, it looks like we are _replacing_ the _start
function from the C runtime, and therefore whoever wrote this code
wanted to do things like the C runtime does.  I see similar code in
crt0.c from the Windows Platform SDK.  If you know why this is dead
code, please tell the details.

> (We appear to have a separate bug with respect to
> nCmdShow: create a shortcut to runemacs.exe and set it to start
> minimized or start maximized. Emacs starts with a normal window.)

The equivalent code in crt0.c indeed looks at dwFlags member of
STARTUPINFO.  So I guess this is evidence that this code does matter,
right?
 
> > And this hunk breaks the MS-DOS build:
> 
> *sigh* The MS-DOS build breaks when the wind blows the wrong way.

That's an exaggeration.  There's no evidence for that.

> Does the build work inside DOSBox? I'll have to start testing the
> build in that environment before pushing.

There's no need.  The DOS build can remain broken on the trunk for
prolonged periods, because no one tracks that.  I usually test the
build at strategic moments, and fix whatever needs fixing.  For this
pretest, I already did that on the branch, and I'd like to keep it
functional if possible.

In general, any introduction of a new variable in src/Makefile.in that
gets replaced by the configure script should be edited by
msdos/sed1v2.inp, usually to an empty value.  But if the configury
stuff doesn't change during a pretest, no changes are needed in
msdos/* files.  Anyway, when in doubt, just ask me, I can test if
needed.

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 10 Dec 2012 09:58:15 -0800
> From: Daniel Colascione <dancol@dancol.org>
> CC: Eli Zaretskii <eliz@gnu.org>, 12993@debbugs.gnu.org, 
>  angelo.graziosi@alice.it
> 
> > Please move your patch from the branch to the trunk, since it doesn't
> > fix such a regression, is not super important (yes, it's definitely
> > nicer to have an icon, but people can live without one until 24.4), and
> > its safety is not obvious.
> 
> Done.

Thank you.

bug#12993: Wrong icon for Cygw32-Emacs

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

On 12/10/2012 11:57 AM, Eli Zaretskii wrote:
>> Date: Mon, 10 Dec 2012 08:24:30 -0800
>> From: Daniel Colascione <dancol@dancol.org>
>> CC: 12993@debbugs.gnu.org, angelo.graziosi@alice.it
>>
>>>        SetErrorMode (SEM_FAILCRITICALERRORS);
>>>     -
>>>     -  /* Invoke the NT CRT startup routine now that our housecleaning
>>>     -     is finished.  */
>>>     -#ifdef HAVE_NTGUI
>>>     -  /* determine WinMain args like crt0.c does */
>>>     -  hinst = GetModuleHandle (NULL);
>>>     -  lpCmdLine = GetCommandLine ();
>>>     -  nCmdShow = SW_SHOWDEFAULT;
>>>     -#endif
>>>        mainCRTStartup ();
>>>      }
>>>
>>> What do you know about lpCmdLine and nCmdShow, and "what crt0.c does"
>>> with them, to be sure they can be removed?
>>
>> I removed dead code. Those variables are never used; the C runtime (to
>> which we dynamically link anyway) handles this initialization
>> internally.
> 
> I'm not sure.  To me, it looks like we are _replacing_ the _start
> function from the C runtime, and therefore whoever wrote this code
> wanted to do things like the C runtime does.  I see similar code in
> crt0.c from the Windows Platform SDK.  If you know why this is dead
> code, please tell the details.

I don't see that code in my copy of VS10's crt0.c.

The code is useless because it doesn't affect program execution. The symbols
lpCmdLine and nCmdShow are defined only because we define them. (If you declare
them extern without defining them, compilation fails.) Therefore, Emacs is the
only user of these variables. The variables aren't used elsewhere in Emacs code
either.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 12/10/2012 2:13 AM, Daniel Colascione wrote:
> On 11/25/12 6:49 AM, Angelo Graziosi wrote:
>> I notice that the Cygw32 port does not show the standard Emacs icon on
>> the upper-left corner (and elsewhere, I think).
>>
>> Would it be possible to have that? The Windows port has...
>
> I've pushed a change to the emacs-24 branch that should resolve the
> problem. We weren't compiling Windows resources into the cygw32 Emacs
> binary.

Daniel,

At some point could you update this fix so that it works on 64-bit 
Cygwin also?  I don't know if you've set up a 64-bit Cygwin environment 
yet.  If not, there's obviously no rush about this.

Thanks.

Ken

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Sun, 07 Apr 2013 14:11:48 -0400
> From: Ken Brown <kbrown@cornell.edu>
> Cc: 12993@debbugs.gnu.org, Angelo Graziosi <angelo.graziosi@alice.it>
> 
> At some point could you update this fix so that it works on 64-bit 
> Cygwin also?

What are the problems that prevent this from working with 64-bit
Cygwin?  I presume there's a 64-bit windres.exe, and nt/emacs.rc is
already set up to cater to 64-bit builds.  What am I missing?

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 4/7/2013 2:48 PM, Eli Zaretskii wrote:
>> Date: Sun, 07 Apr 2013 14:11:48 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>> Cc: 12993@debbugs.gnu.org, Angelo Graziosi <angelo.graziosi@alice.it>
>>
>> At some point could you update this fix so that it works on 64-bit
>> Cygwin also?
>
> What are the problems that prevent this from working with 64-bit
> Cygwin?  I presume there's a 64-bit windres.exe, and nt/emacs.rc is
> already set up to cater to 64-bit builds.  What am I missing?

The problem is that the build fails.  There's no error message 
explaining the failure, just "Makefile:785: recipe for target 
`bootstrap-emacs.exe' failed".  But the build succeeds if I remove the 
attempt to compile Windows resources:

--- configure.ac        2013-03-18 05:26:56 +0000
+++ configure.ac        2013-03-23 15:09:15 +0000
@@ -1524,10 +1525,12 @@
    W32_OBJ="$W32_OBJ w32xfns.o w32select.o w32uniscribe.o"
    W32_LIBS="$W32_LIBS -lkernel32 -luser32 -lgdi32 -lole32 -lcomdlg32"
    W32_LIBS="$W32_LIBS -lusp10 -lcomctl32 -lwinspool"
-  W32_RES="emacs.res"
+  dnl W32_RES="emacs.res"
+  W32_RES=""
    # Tell the linker that emacs.res is an object (which we compile from
    # the rc file), not a linker script.
-  W32_RES_LINK="-Wl,-bpe-i386 -Wl,emacs.res"
+  dnl W32_RES_LINK="-Wl,-bpe-i386 -Wl,emacs.res"
+  W32_RES_LINK=""
  fi
  AC_SUBST(W32_OBJ)
  AC_SUBST(W32_LIBS)


I know nothing about Windows resources, so I have no idea what's causing 
the build failure.

Ken

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Sun, 07 Apr 2013 18:44:36 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: dancol@dancol.org, 12993@debbugs.gnu.org
> 
> On 4/7/2013 2:48 PM, Eli Zaretskii wrote:
> >> Date: Sun, 07 Apr 2013 14:11:48 -0400
> >> From: Ken Brown <kbrown@cornell.edu>
> >> Cc: 12993@debbugs.gnu.org, Angelo Graziosi <angelo.graziosi@alice.it>
> >>
> >> At some point could you update this fix so that it works on 64-bit
> >> Cygwin also?
> >
> > What are the problems that prevent this from working with 64-bit
> > Cygwin?  I presume there's a 64-bit windres.exe, and nt/emacs.rc is
> > already set up to cater to 64-bit builds.  What am I missing?
> 
> The problem is that the build fails.  There's no error message 
> explaining the failure, just "Makefile:785: recipe for target 
> `bootstrap-emacs.exe' failed".  But the build succeeds if I remove the 
> attempt to compile Windows resources:

What is on line 785 of Makefile?

> I know nothing about Windows resources

They are just a fancy kind of object file produced from a fancy kind
of source (emacs.rc in this case).

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 4/7/2013 10:39 PM, Eli Zaretskii wrote:
>> Date: Sun, 07 Apr 2013 18:44:36 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: dancol@dancol.org, 12993@debbugs.gnu.org
>>
>> On 4/7/2013 2:48 PM, Eli Zaretskii wrote:
>>>> Date: Sun, 07 Apr 2013 14:11:48 -0400
>>>> From: Ken Brown <kbrown@cornell.edu>
>>>> Cc: 12993@debbugs.gnu.org, Angelo Graziosi <angelo.graziosi@alice.it>
>>>>
>>>> At some point could you update this fix so that it works on 64-bit
>>>> Cygwin also?
>>>
>>> What are the problems that prevent this from working with 64-bit
>>> Cygwin?  I presume there's a 64-bit windres.exe, and nt/emacs.rc is
>>> already set up to cater to 64-bit builds.  What am I missing?
>>
>> The problem is that the build fails.  There's no error message
>> explaining the failure, just "Makefile:785: recipe for target
>> `bootstrap-emacs.exe' failed".  But the build succeeds if I remove the
>> attempt to compile Windows resources:
>
> What is on line 785 of Makefile?
>
>> I know nothing about Windows resources
>
> They are just a fancy kind of object file produced from a fancy kind
> of source (emacs.rc in this case).

I tried again on a different computer, after updating to the current 
emacs trunk, and this time I got a meaningful error message:

/usr/lib/gcc/x86_64-pc-cygwin/4.8.0/../../../../x86_64-pc-cygwin/bin/ld: 
i386 architecture of input file `emacs.res' is incompatible with 
i386:x86-64 output

This made me think that the problem was the "pe-i386" in the following 
line of configure.ac:

   W32_RES_LINK="-Wl,-bpe-i386 -Wl,emacs.res"

I made several guesses as to what should be there (including 
"pe-x86-64", which seemed the most likely), but I kept getting the same 
error message.  Is it possible that the problem is in nt/emacs.rc after all?

Ken

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 08 Apr 2013 09:02:55 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: dancol@dancol.org, 12993@debbugs.gnu.org
> 
> I tried again on a different computer, after updating to the current 
> emacs trunk, and this time I got a meaningful error message:
> 
> /usr/lib/gcc/x86_64-pc-cygwin/4.8.0/../../../../x86_64-pc-cygwin/bin/ld: 
> i386 architecture of input file `emacs.res' is incompatible with 
> i386:x86-64 output
> 
> This made me think that the problem was the "pe-i386" in the following 
> line of configure.ac:
> 
>    W32_RES_LINK="-Wl,-bpe-i386 -Wl,emacs.res"
> 
> I made several guesses as to what should be there (including 
> "pe-x86-64", which seemed the most likely), but I kept getting the same 
> error message.

What does "objdump -fh emacs.res" say?

> Is it possible that the problem is in nt/emacs.rc after all?

Unlikely, since it works with the 64-bit Microsoft compiler.  Does
Cygwin64 define WIN64?  If not, it might be getting the wrong version
of the manifest.

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 4/8/2013 10:53 AM, Eli Zaretskii wrote:
>> Date: Mon, 08 Apr 2013 09:02:55 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: dancol@dancol.org, 12993@debbugs.gnu.org
>>
>> I tried again on a different computer, after updating to the current
>> emacs trunk, and this time I got a meaningful error message:
>>
>> /usr/lib/gcc/x86_64-pc-cygwin/4.8.0/../../../../x86_64-pc-cygwin/bin/ld:
>> i386 architecture of input file `emacs.res' is incompatible with
>> i386:x86-64 output
>>
>> This made me think that the problem was the "pe-i386" in the following
>> line of configure.ac:
>>
>>     W32_RES_LINK="-Wl,-bpe-i386 -Wl,emacs.res"
>>
>> I made several guesses as to what should be there (including
>> "pe-x86-64", which seemed the most likely), but I kept getting the same
>> error message.
>
> What does "objdump -fh emacs.res" say?

$ objdump -fh src/emacs.res

src/emacs.res:     file format pe-i386
architecture: i386, flags 0x00000039:
HAS_RELOC, HAS_DEBUG, HAS_SYMS, HAS_LOCALS
start address 0x00000000

Sections:
Idx Name          Size      VMA               LMA               File off 
  Algn
   0 .rsrc         0000d5f0  00000000  00000000  0000003c  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, DATA

> > Is it possible that the problem is in nt/emacs.rc after all?
>
> Unlikely, since it works with the 64-bit Microsoft compiler.  Does
> Cygwin64 define WIN64?  If not, it might be getting the wrong version
> of the manifest.

No, it doesn't define WIN64.  It does define __x86_64__ if that helps.

Ken

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 08 Apr 2013 11:25:43 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: dancol@dancol.org, 12993@debbugs.gnu.org
> 
> $ objdump -fh src/emacs.res
> 
> src/emacs.res:     file format pe-i386
> architecture: i386, flags 0x00000039:
> HAS_RELOC, HAS_DEBUG, HAS_SYMS, HAS_LOCALS
> start address 0x00000000
> 
> Sections:
> Idx Name          Size      VMA               LMA               File off 
>   Algn
>    0 .rsrc         0000d5f0  00000000  00000000  0000003c  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, DATA

That doesn't look right, the format shouldn't be pe-i386.  What do you
get for any .o file compiled by the 64-bit Cygwin GCC?

> > > Is it possible that the problem is in nt/emacs.rc after all?
> >
> > Unlikely, since it works with the 64-bit Microsoft compiler.  Does
> > Cygwin64 define WIN64?  If not, it might be getting the wrong version
> > of the manifest.
> 
> No, it doesn't define WIN64.  It does define __x86_64__ if that helps.

Then please try editing emacs.rc to change

  #ifdef WIN64
  1 24 "emacs-x64.manifest"
  #else
  1 24 "emacs-x86.manifest"
  #endif

into

  #ifdef __x86_64__
  1 24 "emacs-x64.manifest"
  #else
  1 24 "emacs-x86.manifest"
  #endif

and see if that helps.

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 4/8/2013 12:24 PM, Eli Zaretskii wrote:
>> Date: Mon, 08 Apr 2013 11:25:43 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: dancol@dancol.org, 12993@debbugs.gnu.org
>>
>> $ objdump -fh src/emacs.res
>>
>> src/emacs.res:     file format pe-i386
>> architecture: i386, flags 0x00000039:
>> HAS_RELOC, HAS_DEBUG, HAS_SYMS, HAS_LOCALS
>> start address 0x00000000
>>
>> Sections:
>> Idx Name          Size      VMA               LMA               File off
>>    Algn
>>     0 .rsrc         0000d5f0  00000000  00000000  0000003c  2**2
>>                     CONTENTS, ALLOC, LOAD, RELOC, DATA
>
> That doesn't look right, the format shouldn't be pe-i386.  What do you
> get for any .o file compiled by the 64-bit Cygwin GCC?

$ objdump -fh src/emacs.o

src/emacs.o:     file format pe-x86-64
architecture: i386:x86-64, flags 0x00000039:
HAS_RELOC, HAS_DEBUG, HAS_SYMS, HAS_LOCALS
start address 0x0000000000000000

Sections:
Idx Name          Size      VMA               LMA               File off 
  Algn
   0 .text         000031c0  0000000000000000  0000000000000000 
0000049c  2**4
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
   1 .data         00000120  0000000000000000  0000000000000000 
0000365c  2**4
                   CONTENTS, ALLOC, LOAD, RELOC, DATA
   2 .bss          00000150  0000000000000000  0000000000000000 
00000000  2**4
                   ALLOC

[...]

>>>> Is it possible that the problem is in nt/emacs.rc after all?
>>>
>>> Unlikely, since it works with the 64-bit Microsoft compiler.  Does
>>> Cygwin64 define WIN64?  If not, it might be getting the wrong version
>>> of the manifest.
>>
>> No, it doesn't define WIN64.  It does define __x86_64__ if that helps.
>
> Then please try editing emacs.rc to change
>
>    #ifdef WIN64
>    1 24 "emacs-x64.manifest"
>    #else
>    1 24 "emacs-x86.manifest"
>    #endif
>
> into
>
>    #ifdef __x86_64__
>    1 24 "emacs-x64.manifest"
>    #else
>    1 24 "emacs-x86.manifest"
>    #endif
>
> and see if that helps.

Yes, the build now completes and the correct icon is there.  Moreover, 
objdump now gives the expected file format:

$ objdump -fr src/emacs.res

src/emacs.res:     file format pe-x86-64
architecture: i386:x86-64, flags 0x00000039:
HAS_RELOC, HAS_DEBUG, HAS_SYMS, HAS_LOCALS
start address 0x0000000000000000

RELOCATION RECORDS FOR [.rsrc]:
OFFSET           TYPE              VALUE
000000000000022c rva32             .rsrc
000000000000023c rva32             .rsrc
000000000000024c rva32             .rsrc
000000000000025c rva32             .rsrc
000000000000026c rva32             .rsrc
000000000000027c rva32             .rsrc
000000000000028c rva32             .rsrc
000000000000029c rva32             .rsrc
00000000000002ac rva32             .rsrc
00000000000002bc rva32             .rsrc
00000000000002cc rva32             .rsrc
00000000000002dc rva32             .rsrc

Thank you!  So I assume the correct fix is the following:

=== modified file 'nt/emacs.rc'
--- nt/emacs.rc 2013-03-05 03:59:35 +0000
+++ nt/emacs.rc 2013-04-08 16:55:12 +0000
@@ -1,6 +1,6 @@
  Emacs ICON   icons/emacs.ico
  32649 CURSOR icons/hand.cur
-#ifdef WIN64
+#if defined (WIN64) || defined (__x86_64__)
  1 24 "emacs-x64.manifest"
  #else
  1 24 "emacs-x86.manifest"

Right?

Ken

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 08 Apr 2013 12:58:40 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: dancol@dancol.org, 12993@debbugs.gnu.org
> 
> Thank you!  So I assume the correct fix is the following:
> 
> === modified file 'nt/emacs.rc'
> --- nt/emacs.rc 2013-03-05 03:59:35 +0000
> +++ nt/emacs.rc 2013-04-08 16:55:12 +0000
> @@ -1,6 +1,6 @@
>   Emacs ICON   icons/emacs.ico
>   32649 CURSOR icons/hand.cur
> -#ifdef WIN64
> +#if defined (WIN64) || defined (__x86_64__)
>   1 24 "emacs-x64.manifest"
>   #else
>   1 24 "emacs-x86.manifest"
> 
> Right?

Yes.  But what about that linker directive in configure.ac -- is that
still required?  (I never understood why Cygwin needs that in the
first place.)

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 4/8/2013 1:20 PM, Eli Zaretskii wrote:
>> Date: Mon, 08 Apr 2013 12:58:40 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: dancol@dancol.org, 12993@debbugs.gnu.org
>>
>> Thank you!  So I assume the correct fix is the following:
>>
>> === modified file 'nt/emacs.rc'
>> --- nt/emacs.rc 2013-03-05 03:59:35 +0000
>> +++ nt/emacs.rc 2013-04-08 16:55:12 +0000
>> @@ -1,6 +1,6 @@
>>    Emacs ICON   icons/emacs.ico
>>    32649 CURSOR icons/hand.cur
>> -#ifdef WIN64
>> +#if defined (WIN64) || defined (__x86_64__)
>>    1 24 "emacs-x64.manifest"
>>    #else
>>    1 24 "emacs-x86.manifest"
>>
>> Right?
>
> Yes.  But what about that linker directive in configure.ac -- is that
> still required?  (I never understood why Cygwin needs that in the
> first place.)

The "-Wl,-bpe-i386" part is not necessary.  I just checked this on both 
Cygwin32 and Cygwin64.  I think it should be removed, since it's 
confusing to have an unnecessary directive (especially on Cygwin64, 
where the format is actually pe-x86-64).  But if both directives are 
removed, the icon doesn't show up.  I guess the comment right before 
that line in configure.ac explains why.

Ken

bug#12993: Wrong icon for Cygw32-Emacs

[Eli Zaretskii]

> Date: Mon, 08 Apr 2013 16:26:39 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: dancol@dancol.org, 12993@debbugs.gnu.org
> 
> The "-Wl,-bpe-i386" part is not necessary.  I just checked this on both 
> Cygwin32 and Cygwin64.  I think it should be removed, since it's 
> confusing to have an unnecessary directive (especially on Cygwin64, 
> where the format is actually pe-x86-64).  But if both directives are 
> removed, the icon doesn't show up.  I guess the comment right before 
> that line in configure.ac explains why.

Then you will have to provide a different directive for the 64-bit
build, because the file type is no longer pe-i386.

bug#12993: Wrong icon for Cygw32-Emacs

[Ken Brown]

On 4/8/2013 4:26 PM, Ken Brown wrote:
> On 4/8/2013 1:20 PM, Eli Zaretskii wrote:
>>> Date: Mon, 08 Apr 2013 12:58:40 -0400
>>> From: Ken Brown <kbrown@cornell.edu>
>>> CC: dancol@dancol.org, 12993@debbugs.gnu.org
>>>
>>> Thank you!  So I assume the correct fix is the following:
>>>
>>> === modified file 'nt/emacs.rc'
>>> --- nt/emacs.rc 2013-03-05 03:59:35 +0000
>>> +++ nt/emacs.rc 2013-04-08 16:55:12 +0000
>>> @@ -1,6 +1,6 @@
>>>    Emacs ICON   icons/emacs.ico
>>>    32649 CURSOR icons/hand.cur
>>> -#ifdef WIN64
>>> +#if defined (WIN64) || defined (__x86_64__)
>>>    1 24 "emacs-x64.manifest"
>>>    #else
>>>    1 24 "emacs-x86.manifest"
>>>
>>> Right?
>>
>> Yes.  But what about that linker directive in configure.ac -- is that
>> still required?  (I never understood why Cygwin needs that in the
>> first place.)
>
> The "-Wl,-bpe-i386" part is not necessary.  I just checked this on both
> Cygwin32 and Cygwin64.  I think it should be removed, since it's
> confusing to have an unnecessary directive (especially on Cygwin64,
> where the format is actually pe-x86-64).  But if both directives are
> removed, the icon doesn't show up.  I guess the comment right before
> that line in configure.ac explains why.

I've removed "-Wl,-bpe-i386" and made the change to nt/emacs.rc as bzr 
revision 112251.  I'm closing the bug report.  Thanks again for your 
help, Eli.

Ken