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

["Bunk, Bernd" <bernd dot bunk at intel dot com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, August 23, 2013 4:39 PM
> To: Bunk, Bernd
> Cc: Eli Zaretskii; tromey@redhat.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2] Added file properties to windows gdb executable
> for all mingw32 builds.
> 
> 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?
I can offer to merge this icon into my mingw rc file in a later checkin.  But tbh I cannot even find the sources (icon or gdbtk folder).  Someone checked in build rules for something that is not on the same branch (upstream/master).  I am new to this gdb coding, but I would never do something like this...

> 
> >
> >>
> >> 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".)
Confused me also a bit, but I got it and changed the comments.  The reason why there was no problem with comment styles in create-version.sh is, that there are no comments in it (besides copyright header).  Anyway, I fixed this in v3.

> 
> >
> >>
> >>> +   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@.
> 
Thanks for these links.  I will check this out once the diff has been upstreamed and update it.

> >>
> >>> +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.
I implemented it slightly different [see v3].  The script that generates the header file now generates it in the build folder (instead of source folder), and the build rule now adds "-I." to the $(WINDRES) call, so that the (local) header file gets found during build.  No need to copy anything here.  But I am flexible, if you prefer that I build a copy of the rc file I am also fine with it.

> 
> --
> Pedro Alves
> 

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052