Re: [PATCH v2] Added file properties to windows gdb executable for all mingw32 builds.

[Pedro Alves <palves@redhat.com>]

On 08/23/2013 03:05 PM, Bunk, Bernd wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Thursday, August 22, 2013 9:24 PM
>> To: Eli Zaretskii
>> Cc: Bunk, Bernd; tromey@redhat.com; gdb-patches@sourceware.org
>> Subject: Re: [PATCH v2] Added file properties to windows gdb executable
>> for all mingw32 builds.
>>
>> On 08/22/2013 04:09 PM, Eli Zaretskii wrote:
>>
>>> The GNU project doesn't like calling Windows a "win", so I suggest to
>>> rename the files and the script to use something like mingw instead.
>>
>> I'd think this works just as well on Cygwin binaries?  I suggest
>> windows-*.
> I did not implement this for Cygwin for several reasons:
> 
> First I have no means to test that here and did not want to submit untested features or even break other builds.
> 
> And second I detected that there is a Cygwin based build (gdbtk) that has a resource (with an icon) build and linked into a windows gdb binary.  I did not want to interfere with that (again no way to test it).
> 
> I could extend my implementation to all windows host builds, but TBH setting up a Cygwin environment (inside our build environment) is a huge effort that is overkill for this little feature.
> 
>>
>> "properties" sounds odd to me, given files are usually called resource
>> files.  IIRC, the frequent file name used in lots of projects is
>> winres.rc.  But then, if you have more than one program in your
>> project, you'd go with gdbres.rc, foores.rc, etc.  Which leads me to
>> ...  We already have some support for including resources (icon, etc.)
>> in the binary, for gdbtk.
>> See gdb/configure.ac, gdb/Makefile.in and gdbtk/gdb.rc etc.
>> Isn't this going to conflict on gdbtk builds?
> See above, yes it would.

Well, it just looks to me that the gdbres.rc resource
and the icon are presently only used on Cygwin-hosts builds,
because nobody bothered to tweak that configure bit to make it work
on mingw-hosts too.  That bit is there since at least 2000, and I
believe MinGW host support came in much after.  Cygwin wanting
pretty icons, but MinGW not is kind of strange, wouldn't
you say?  :-)

Do you have any hint at what one would need to do, if one
were to enable that icon resource on mingw too?

> 
>>
>> While at it, follows a review of the patch.
>>
>>> +# shell parameters
>> ...
>>
>>> +
>>> +# default option values
>>> +# keep these defaults in sync with gdb/top.c print_gdb_version()
>>
>> ...
>>
>>> +# check for environment variables to replace certain file properties
>>
>> Please make the comments follow the GNU style.  That is, Start
>> sentences with upper case, and end them with a double period.
> Ok, I found only one shell script in that area (common/create-version.sh) and used that as code example.  But I can adapt the comment/coding style - no problem [see next update]

I don't see any comment formatting problem in create-version.sh.

(btw, I meant "double space after period".  Don't know
why I wrote "double period".)

> 
>>
>>> +   Patch this header file using create-win_exe_properties.sh during
>> gdb build
>>> +   to customize the file properties. */
>>
>> Likewise, double period.  There may be more instances.
>>
>>> +# keep these defaults in sync with gdb/top.c print_gdb_version()
>> ...
>>> +copyright="Copyright (C) 2013 Free Software Foundation, Inc."
>>
>> That "keep in sync" comment is useless, as nobody will remember to look
>> here when looking at top.c.  Instead, remove that, and you'll need to
>> update the "Start of New Year Procedure" section in the internals
>> manual.  Guess that means in the wiki now, once this patch is in.
> Ok, I remove that command, but will need to investigate what exactly this means.  But changing a wiki afterwards does not sound so difficult.
> 

I mean this section:

 http://sourceware.org/gdb/current/onlinedocs/gdbint/Start-of-New-Year-Procedure.html#Start-of-New-Year-Procedure

The internals manual has just recently all been dumped into the wiki:

  http://sourceware.org/gdb/wiki/Internals%20Start-of-New-Year-Procedure

See "A new strategy for internals documentation" thread in gdb@.

>>
>>> +win_exe_properties.h: Makefile version.in common/create-
>> win_exe_properties.sh
>>> +	$(SHELL) $(srcdir)/common/create-win_exe_properties.sh $(srcdir)
>> \
>>> +		"$(host_alias)" "$(target_alias)" win_exe_properties.h
>>
>> It's better to write to a temporary file, and then move to the final
>> destination, so you don't end up with a half baked file if you cancel
>> the build at the wrong time.
> Ok, didn't think about this - good catch!  So I will follow the version file approach and create a temp (header) file inside the build folder, not inside the $(srcdir) folder. [see next update]

Yes, see e.g., the copying.c rule just above the rule you added.

-- 
Pedro Alves