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

["Bunk, Bernd" <bernd.bunk@intel.com>]

> -----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.

> 
> 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]

> 
> > +   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.

> 
> > +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]

> 
> --
> 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