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

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

[Bernd Bunk]

On Windows OS it is customary that executables and DLL's show file properties.
These contain product name/version, file name/version, company/copyright info.
File properties are visible by right-click on the file, selecting properties \ details.
This patch adds file properties to the gdb executable for all mingw builds.

2013-08-14  Bernd Bunk  <bernd.bunk@intel.com>

	* Makefile.in (win_exe_properties.h): Add rule to create
	win_exe_properties.h header file with file property information.
	(win_exe_properties.o): Added rule to build the resource file.
	* configure: Add win_exe_properties.o to mingw32 specific
	objects.
	* common/create-win_exe_properties.sh: Shell script to
	create a win_exe_properties.h header file containing all
	information/defines for the gdb executable file properties.
	New file.
	* win_exe_properties.rc: Resource file implementing
	gdb executable file properties for windows/mingw32 builds.
	It includes auto-generated win_exe_properties.h file with
	property information. New file.

Signed-off-by: Bernd Bunk <bernd.bunk@intel.com>

---
 gdb/Makefile.in                         |    7 ++
 gdb/common/create-win_exe_properties.sh |  115 +++++++++++++++++++++++++++++++
 gdb/configure                           |    9 +++
 gdb/win_exe_properties.rc               |   61 ++++++++++++++++
 4 files changed, 192 insertions(+), 0 deletions(-)
 create mode 100644 gdb/common/create-win_exe_properties.sh
 create mode 100644 gdb/win_exe_properties.rc

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 45cddaf..425793f 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1426,6 +1426,13 @@ $(srcdir)/copying.c: @MAINTAINER_MODE_TRUE@ $(srcdir)/../COPYING3 $(srcdir)/copy
 		< $(srcdir)/../COPYING3 > $(srcdir)/copying.tmp
 	mv $(srcdir)/copying.tmp $(srcdir)/copying.c
 
+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
+
+win_exe_properties.o: win_exe_properties.h win_exe_properties.rc
+	$(WINDRES) $(srcdir)/win_exe_properties.rc win_exe_properties.o
+
 version.c: Makefile version.in $(srcdir)/../bfd/version.h $(srcdir)/common/create-version.sh
 	$(SHELL) $(srcdir)/common/create-version.sh $(srcdir) \
 	    $(host_alias) $(target_alias) version.c
diff --git a/gdb/common/create-win_exe_properties.sh b/gdb/common/create-win_exe_properties.sh
new file mode 100644
index 0000000..4942024
--- /dev/null
+++ b/gdb/common/create-win_exe_properties.sh
@@ -0,0 +1,115 @@
+#!/bin/sh
+
+# Create windows executable file properties for GDB on Windows.
+# These are visible in context menu Properties / Details on the GDB
+# executable on Windows.
+#
+# Copyright (C) 2013 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corporation
+#
+# This file is part of GDB.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Create win_exe_properties.h from given parameters
+# Usage:
+#    create-win_exe_properties.sh PATH-TO-GDB-SRCDIR HOST_ALIAS \
+#        TARGET_ALIAS OUTPUT-FILE-NAME OPTIONS
+#
+#    see also environment variable below to customize some of the
+#    description fields.
+#
+
+# shell parameters
+srcdir="$1"
+host_alias="$2"
+target_alias="$3"
+output="$4"
+
+if [ $# -ne 4 ] ; then
+  echo "usage: $0 PATH-TO-GDB-SRCDIR HOST_ALIAS TARGET_ALIAS OUTPUT-FILE-NAME" >&2
+  exit 1
+fi
+
+# default option values
+# keep these defaults in sync with gdb/top.c print_gdb_version()
+version=`cat $srcdir/version.in`
+company_name="Free Software Foundation, Inc."
+file_description="gdb"
+product_name="gdb"
+configured=""
+if [ -n "$host_alias" ] ; then
+  file_description="gdb for $host_alias"
+  product_name="gdb for $host_alias"
+  configured="This GDB was configured as \"\"$host_alias\"\"."
+fi
+internal_name="gdb.exe"
+original_filename="gdb.exe"
+copyright="Copyright (C) 2013 Free Software Foundation, Inc."
+  license=\
+"License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
+This is free software: you are free to change and redistribute it.
+There is NO WARRANTY, to the extent permitted by law.
+Type \"\"show copying\"\" and \"\"show warranty\"\" for details."
+support="For bug reporting instructions, please see:
+<http://www.gnu.org/software/gdb/bugs/>."
+
+# check for environment variables to replace certain file properties
+[ -n "$WIN_EXE_VERSION" ] && version=$WIN_EXE_VERSION
+[ -n "$WIN_EXE_COMPANY_NAME" ] && company_name=$WIN_EXE_COMPANY_NAME
+[ -n "$WIN_EXE_FILE_DESCRIPTION" ] && file_description=$WIN_EXE_FILE_DESCRIPTION
+[ -n "$WIN_EXE_PRODUCT_NAME" ] && product_name=$WIN_EXE_PRODUCT_NAME
+[ -n "$WIN_EXE_INTERNAL_NAME" ] && internal_name=$WIN_EXE_INTERNAL_NAME
+[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] && original_filename=$WIN_EXE_ORIGINAL_FILENAME
+[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT
+[ -n "$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE
+[ -n "$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED
+[ -n "$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT
+
+# combine complete copyright message
+# Windows file property dialog only shows ONE line (with scrollbar)
+# so do not separate with newlines but use spaces instead!
+copyright_all=`echo $copyright $license $configured $support`
+
+# remove "#" at the beginning and internal version at the end
+version=`echo ${version%-*} | sed -e "s/#//"`
+
+# split version
+IFS_BAK="$IFS"
+IFS='.'
+array=($version)
+version_major=${array[0]}
+version_minor=${array[1]}
+version_build=${array[2]}
+IFS="$IFS_BAK"
+[ -z "$version_major" ] && version_major=0
+[ -z "$version_minor" ] && version_minor=0
+[ -z "$version_build" ] && version_build=0
+
+# write version into output file
+echo "#define FP_VERSION_MAJOR $version_major"  > $srcdir/$output
+echo "#define FP_VERSION_MINOR $version_minor" >> $srcdir/$output
+echo "#define FP_VERSION_BUILD $version_build" >> $srcdir/$output
+echo "#define FP_VERSION_ALL   \"$version_major.$version_minor.$version_build\"" >> $srcdir/$output
+
+echo "" >> $srcdir/$output
+
+# write other file properties into output file
+echo "#define FP_COMPANY_NAME      \"$company_name\""      >> $srcdir/$output
+echo "#define FP_FILE_DESCRIPTION  \"$file_description\""  >> $srcdir/$output
+echo "#define FP_INTERNAL_NAME     \"$internal_name\""     >> $srcdir/$output
+echo "#define FP_COPYRIGHT         \"$copyright_all\""     >> $srcdir/$output
+echo "#define FP_ORIGINAL_FILENAME \"$original_filename\"" >> $srcdir/$output
+echo "#define FP_PRODUCT_NAME      \"$product_name\""      >> $srcdir/$output
diff --git a/gdb/configure b/gdb/configure
index 8067825..5afea6a 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -6988,6 +6988,15 @@ case $host_os in
     ;;
 esac
 
+# If we build for mingw32/Windows, then also build file properties
+# for GDB executable.
+case $host_os in
+  *mingw32*)
+    CONFIG_OBS="$CONFIG_OBS win_exe_properties.o"
+    ;;
+esac
+
+
 # These are the libraries checked by Readline.
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing tgetent" >&5
 $as_echo_n "checking for library containing tgetent... " >&6; }
diff --git a/gdb/win_exe_properties.rc b/gdb/win_exe_properties.rc
new file mode 100644
index 0000000..270c04c
--- /dev/null
+++ b/gdb/win_exe_properties.rc
@@ -0,0 +1,61 @@
+/* Windows executable properties for GDB on Windows.
+   These are visible in context menu Properties / Details on the GDB
+   executable on Windows.
+
+   Copyright (C) 2013 Free Software Foundation, Inc.
+
+   Contributed by Intel Corporation.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Value defines for windows executable file properties of GDB on Windows.
+   Patch this header file using create-win_exe_properties.sh during gdb build
+   to customize the file properties. */
+#include "win_exe_properties.h"
+
+#include "afxres.h"
+VS_VERSION_INFO VERSIONINFO
+  FILEVERSION FP_VERSION_MAJOR,FP_VERSION_MINOR,FP_VERSION_BUILD,0
+  PRODUCTVERSION FP_VERSION_MAJOR,FP_VERSION_MINOR,FP_VERSION_BUILD,0
+  FILEFLAGSMASK 0x3fL
+#ifdef _DEBUG
+  FILEFLAGS 0x1L
+#else
+  FILEFLAGS 0x0L
+#endif
+  FILEOS 0x40004L
+  FILETYPE 0x1L
+  FILESUBTYPE 0x0L
+BEGIN
+  BLOCK "StringFileInfo"
+  BEGIN
+    BLOCK "040904b0"
+    BEGIN
+      VALUE "CompanyName", FP_COMPANY_NAME
+      VALUE "FileDescription", FP_FILE_DESCRIPTION
+      VALUE "FileVersion", FP_VERSION_ALL
+      VALUE "InternalName", FP_INTERNAL_NAME
+      VALUE "LegalCopyright", FP_COPYRIGHT
+      VALUE "OriginalFilename", FP_ORIGINAL_FILENAME
+      VALUE "ProductName", FP_PRODUCT_NAME
+      VALUE "ProductVersion", FP_VERSION_ALL
+    END
+  END
+  BLOCK "VarFileInfo"
+  BEGIN
+    VALUE "Translation", 0x409, 1200
+  END
+END
-- 
1.7.1

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

[Eli Zaretskii]

> From: Bernd Bunk <bernd.bunk@intel.com>
> Cc: gdb-patches@sourceware.org, Bernd Bunk <bernd.bunk@intel.com>
> Date: Thu, 22 Aug 2013 10:56:06 +0200
> 
> On Windows OS it is customary that executables and DLL's show file properties.
> These contain product name/version, file name/version, company/copyright info.
> File properties are visible by right-click on the file, selecting properties \ details.
> This patch adds file properties to the gdb executable for all mingw builds.

Thanks, this looks OK to me, except for a couple of comments:

> 	* Makefile.in (win_exe_properties.h): Add rule to create
> 	win_exe_properties.h header file with file property information.
> 	(win_exe_properties.o): Added rule to build the resource file.
> 	* configure: Add win_exe_properties.o to mingw32 specific
> 	objects.
> 	* common/create-win_exe_properties.sh: Shell script to
> 	create a win_exe_properties.h header file containing all
> 	information/defines for the gdb executable file properties.
> 	New file.
> 	* win_exe_properties.rc: Resource file implementing
> 	gdb executable file properties for windows/mingw32 builds.
> 	It includes auto-generated win_exe_properties.h file with
> 	property information. New file.

New files you add should say "New file".  Also, please leave two
spaces between sentences, the US style.

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.

> +# check for environment variables to replace certain file properties
> +[ -n "$WIN_EXE_VERSION" ] && version=$WIN_EXE_VERSION
> +[ -n "$WIN_EXE_COMPANY_NAME" ] && company_name=$WIN_EXE_COMPANY_NAME
> +[ -n "$WIN_EXE_FILE_DESCRIPTION" ] && file_description=$WIN_EXE_FILE_DESCRIPTION
> +[ -n "$WIN_EXE_PRODUCT_NAME" ] && product_name=$WIN_EXE_PRODUCT_NAME
> +[ -n "$WIN_EXE_INTERNAL_NAME" ] && internal_name=$WIN_EXE_INTERNAL_NAME
> +[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] && original_filename=$WIN_EXE_ORIGINAL_FILENAME
> +[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT
> +[ -n "$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE
> +[ -n "$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED
> +[ -n "$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT

This looks like unnecessary featurism to me.  Is it really needed, and
if so, in what use cases?
> +#include "afxres.h"

Is this header really needed?

Thanks.

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

[Pedro Alves]

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

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

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.

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

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

-- 
Pedro Alves

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

[Eli Zaretskii]

> Date: Thu, 22 Aug 2013 20:24:20 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: Bernd Bunk <bernd.bunk@intel.com>, tromey@redhat.com,        gdb-patches@sourceware.org
> 
> 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?

Not as submitted:

> +# If we build for mingw32/Windows, then also build file properties
> +# for GDB executable.
> +case $host_os in
> +  *mingw32*)
> +    CONFIG_OBS="$CONFIG_OBS win_exe_properties.o"
> +    ;;
> +esac

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

[Pedro Alves]

On 08/22/2013 08:55 PM, Eli Zaretskii wrote:
>> Date: Thu, 22 Aug 2013 20:24:20 +0100
>> From: Pedro Alves <palves@redhat.com>
>> CC: Bernd Bunk <bernd.bunk@intel.com>, tromey@redhat.com,        gdb-patches@sourceware.org
>>
>> 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?
> 
> Not as submitted:

Right, I mean, other than the fact that the patch only
enabled this on mingw, there's nothing in the feature
itself that'd prevent it from working on Cygwin, afaik,
as this is a Windows binary thing, not specific of
MinGW's toolchain.

-- 
Pedro Alves

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

[Eli Zaretskii]

> Date: Fri, 23 Aug 2013 14:06:51 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: bernd.bunk@intel.com, tromey@redhat.com, gdb-patches@sourceware.org
> 
> On 08/22/2013 08:55 PM, Eli Zaretskii wrote:
> >> Date: Thu, 22 Aug 2013 20:24:20 +0100
> >> From: Pedro Alves <palves@redhat.com>
> >> CC: Bernd Bunk <bernd.bunk@intel.com>, tromey@redhat.com,        gdb-patches@sourceware.org
> >>
> >> 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?
> > 
> > Not as submitted:
> 
> Right, I mean, other than the fact that the patch only
> enabled this on mingw, there's nothing in the feature
> itself that'd prevent it from working on Cygwin, afaik,
> as this is a Windows binary thing, not specific of
> MinGW's toolchain.

The question is: do the Cygwin maintainers _want_ the Cygwin GDB have
all these resources linked into the binary.  Cygwin is a Posix
environment, where these attributes are neither expected nor (AFAIK)
used in any way.

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

[Pedro Alves]

On 08/23/2013 02:27 PM, Eli Zaretskii wrote:
>> Date: Fri, 23 Aug 2013 14:06:51 +0100
>> From: Pedro Alves <palves@redhat.com>
>> Right, I mean, other than the fact that the patch only
>> enabled this on mingw, there's nothing in the feature
>> itself that'd prevent it from working on Cygwin, afaik,
>> as this is a Windows binary thing, not specific of
>> MinGW's toolchain.
> 
> The question is: do the Cygwin maintainers _want_ the Cygwin GDB have
> all these resources linked into the binary.  Cygwin is a Posix
> environment, where these attributes are neither expected nor (AFAIK)
> used in any way.

I'm really just pointing out Cygwin as rationale for preferring
windows-*.c as file name instead of mingw-*.  In my mind, given
this is a Windows/PE feature, not specific of mingw, the file
naming would reflect that.  That is, name the files for what
they are, not for how they're used today.  If the Cygwin guys at
some point want to link in the resources, then it'd be a no
brainer to tweak configure.  If the files are called mingw-*, then such
a change would be kind of awkward without a file rename.  But this
is really bike shedding, not worth spending more cycles on it.  If
you guys prefer mingw, then let's go with it.

-- 
Pedro Alves

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

[Bunk, Bernd]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Thursday, August 22, 2013 5:10 PM
> To: Bunk, Bernd
> Cc: tromey@redhat.com; palves@redhat.com; gdb-patches@sourceware.org;
> Bunk, Bernd
> Subject: Re: [PATCH v2] Added file properties to windows gdb executable
> for all mingw32 builds.
> 
> > From: Bernd Bunk <bernd.bunk@intel.com>
> > Cc: gdb-patches@sourceware.org, Bernd Bunk <bernd.bunk@intel.com>
> > Date: Thu, 22 Aug 2013 10:56:06 +0200
> >
> > On Windows OS it is customary that executables and DLL's show file
> properties.
> > These contain product name/version, file name/version,
> company/copyright info.
> > File properties are visible by right-click on the file, selecting
> properties \ details.
> > This patch adds file properties to the gdb executable for all mingw
> builds.
> 
> Thanks, this looks OK to me, except for a couple of comments:
> 
> > 	* Makefile.in (win_exe_properties.h): Add rule to create
> > 	win_exe_properties.h header file with file property information.
> > 	(win_exe_properties.o): Added rule to build the resource file.
> > 	* configure: Add win_exe_properties.o to mingw32 specific
> > 	objects.
> > 	* common/create-win_exe_properties.sh: Shell script to
> > 	create a win_exe_properties.h header file containing all
> > 	information/defines for the gdb executable file properties.
> > 	New file.
> > 	* win_exe_properties.rc: Resource file implementing
> > 	gdb executable file properties for windows/mingw32 builds.
> > 	It includes auto-generated win_exe_properties.h file with
> > 	property information. New file.
> 
> New files you add should say "New file".  Also, please leave two spaces
> between sentences, the US style.
Ok, I will remove the text and only write "New file." for the 2 new files [see next update].

> 
> 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.
Ok, I will rename it to mingw, since this change is only for mingw.
(see also discsuion coming in a later email)

> 
> > +# check for environment variables to replace certain file properties
> > +[ -n "$WIN_EXE_VERSION" ] && version=$WIN_EXE_VERSION [ -n
> > +"$WIN_EXE_COMPANY_NAME" ] && company_name=$WIN_EXE_COMPANY_NAME
> > +[ -n "$WIN_EXE_FILE_DESCRIPTION" ] &&
> > +file_description=$WIN_EXE_FILE_DESCRIPTION
> > +[ -n "$WIN_EXE_PRODUCT_NAME" ] && product_name=$WIN_EXE_PRODUCT_NAME
> > +[ -n "$WIN_EXE_INTERNAL_NAME" ] &&
> > +internal_name=$WIN_EXE_INTERNAL_NAME
> > +[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] &&
> > +original_filename=$WIN_EXE_ORIGINAL_FILENAME
> > +[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT [ -n
> > +"$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE [ -n
> > +"$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED [ -n
> > +"$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT
> 
> This looks like unnecessary featurism to me.  Is it really needed, and
> if so, in what use cases?
Yes, it is needed. Not in here, but for every company which changes/adds and re-distributes gdb.
I started this feature because our Product Validation does not like binaries without legal information.
And off course this is different depending on who ships the product.
Without a way to change the strings the complete changeset would be useless for me.

> > +#include "afxres.h"
> 
> Is this header really needed?
Yes, unfortunately.
This header file "implements" the resource language used below in the RC file.

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

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

[Bunk, Bernd]

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

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

[Bunk, Bernd]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, August 23, 2013 3:42 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/23/2013 02:27 PM, Eli Zaretskii wrote:
> >> Date: Fri, 23 Aug 2013 14:06:51 +0100
> >> From: Pedro Alves <palves@redhat.com> Right, I mean, other than the
> >> fact that the patch only enabled this on mingw, there's nothing in
> >> the feature itself that'd prevent it from working on Cygwin, afaik,
> >> as this is a Windows binary thing, not specific of MinGW's
> toolchain.
> >
> > The question is: do the Cygwin maintainers _want_ the Cygwin GDB have
> > all these resources linked into the binary.  Cygwin is a Posix
> > environment, where these attributes are neither expected nor (AFAIK)
> > used in any way.
> 
> I'm really just pointing out Cygwin as rationale for preferring
> windows-*.c as file name instead of mingw-*.  In my mind, given this is
> a Windows/PE feature, not specific of mingw, the file naming would
> reflect that.  That is, name the files for what they are, not for how
> they're used today.  If the Cygwin guys at some point want to link in
> the resources, then it'd be a no brainer to tweak configure.  If the
> files are called mingw-*, then such a change would be kind of awkward
> without a file rename.  But this is really bike shedding, not worth
> spending more cycles on it.  If you guys prefer mingw, then let's go
> with it.
Ok, I am also fine with windows-*, as long as I am not expected to do this Cygwin adaptation.  If they want this feature they will have it easy and they have the means to test it.  Perfect for both parties.  I will now rework the code according to the feedback from the last discussions.

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

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

[asmwarrior]

On 2013-8-23 21:44, Bunk, Bernd wrote:
>>> > > +#include "afxres.h"
>> > 
>> > Is this header really needed?
> Yes, unfortunately.
> This header file "implements" the resource language used below in the RC file.
> 

I looked at afxres.h file under a mingw distribution (GCC 4.6.3 with MinGW-W64 API), its contents is below

/**
 * This file has no copyright assigned and is placed in the Public Domain.
 * This file is part of the w64 mingw-runtime package.
 * No warranty is given; refer to the file DISCLAIMER.PD within this package.
 */
#ifndef _AFXRES_H
#define _AFXRES_H
#if __GNUC__ >= 3
#pragma GCC system_header
#endif

#ifdef __cplusplus
extern "C" {
#endif

#ifndef _INC_WINDOWS
#include <windows.h>
#endif

/* IDC_STATIC is documented in winuser.h, but not defined. */
#ifndef IDC_STATIC
#define IDC_STATIC (-1)
#endif

#ifdef __cplusplus
}
#endif
#endif



Look, it just includes windows.h.


Yuanhui Zhang

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

[Eli Zaretskii]

> From: "Bunk, Bernd" <bernd.bunk@intel.com>
> CC: "tromey@redhat.com" <tromey@redhat.com>, "palves@redhat.com"
> 	<palves@redhat.com>, "gdb-patches@sourceware.org"
> 	<gdb-patches@sourceware.org>
> Date: Fri, 23 Aug 2013 13:44:58 +0000
> 
> > > +# check for environment variables to replace certain file properties
> > > +[ -n "$WIN_EXE_VERSION" ] && version=$WIN_EXE_VERSION [ -n
> > > +"$WIN_EXE_COMPANY_NAME" ] && company_name=$WIN_EXE_COMPANY_NAME
> > > +[ -n "$WIN_EXE_FILE_DESCRIPTION" ] &&
> > > +file_description=$WIN_EXE_FILE_DESCRIPTION
> > > +[ -n "$WIN_EXE_PRODUCT_NAME" ] && product_name=$WIN_EXE_PRODUCT_NAME
> > > +[ -n "$WIN_EXE_INTERNAL_NAME" ] &&
> > > +internal_name=$WIN_EXE_INTERNAL_NAME
> > > +[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] &&
> > > +original_filename=$WIN_EXE_ORIGINAL_FILENAME
> > > +[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT [ -n
> > > +"$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE [ -n
> > > +"$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED [ -n
> > > +"$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT
> > 
> > This looks like unnecessary featurism to me.  Is it really needed, and
> > if so, in what use cases?
> Yes, it is needed. Not in here, but for every company which changes/adds and re-distributes gdb.
> I started this feature because our Product Validation does not like binaries without legal information.
> And off course this is different depending on who ships the product.
> Without a way to change the strings the complete changeset would be useless for me.

You can always modify the source of these attributes, can't you?  It's
not like you change these strings several times a day, right?

> > > +#include "afxres.h"
> > 
> > Is this header really needed?
> Yes, unfortunately.
> This header file "implements" the resource language used below in the RC file.

Sorry, I don't understand: this header file in MinGW distribution just
includes windows.h, defines IDC_STATIC, and that's it.  What do you
have in it?

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

[Pedro Alves]

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

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

[Bunk, Bernd]

> -----Original Message-----
> From: asmwarrior [mailto:asmwarrior@gmail.com]
> Sent: Friday, August 23, 2013 4:27 PM
> To: Bunk, Bernd
> Cc: Eli Zaretskii; tromey@redhat.com; palves@redhat.com; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v2] Added file properties to windows gdb executable
> for all mingw32 builds.
> 
> On 2013-8-23 21:44, Bunk, Bernd wrote:
> >>> > > +#include "afxres.h"
> >> >
> >> > Is this header really needed?
> > Yes, unfortunately.
> > This header file "implements" the resource language used below in the
> RC file.
> >
> 
> I looked at afxres.h file under a mingw distribution (GCC 4.6.3 with
> MinGW-W64 API), its contents is below
Ok, my apologies for the wrong assumption.  In 13 years Windows GUI programming I haven't seen an RC file without this afxres.h include.  But I just tested it and it works fine with windows.h.  It even compiles and links without any include, but then the complete file properties are empty at the end.  I will include windows.h. [see next update]

> 
> /**
>  * This file has no copyright assigned and is placed in the Public
> Domain.
>  * This file is part of the w64 mingw-runtime package.
>  * No warranty is given; refer to the file DISCLAIMER.PD within this
> package.
>  */
> #ifndef _AFXRES_H
> #define _AFXRES_H
> #if __GNUC__ >= 3
> #pragma GCC system_header
> #endif
> 
> #ifdef __cplusplus
> extern "C" {
> #endif
> 
> #ifndef _INC_WINDOWS
> #include <windows.h>
> #endif
> 
> /* IDC_STATIC is documented in winuser.h, but not defined. */ #ifndef
> IDC_STATIC #define IDC_STATIC (-1) #endif
> 
> #ifdef __cplusplus
> }
> #endif
> #endif
> 
> 
> 
> Look, it just includes windows.h.
> 
> 
> Yuanhui Zhang
> 

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

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

[Bunk, Bernd]

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

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

[Bunk, Bernd]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Friday, August 23, 2013 4:37 PM
> To: Bunk, Bernd
> Cc: tromey@redhat.com; palves@redhat.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2] Added file properties to windows gdb executable
> for all mingw32 builds.
> 
> > From: "Bunk, Bernd" <bernd.bunk@intel.com>
> > CC: "tromey@redhat.com" <tromey@redhat.com>, "palves@redhat.com"
> > 	<palves@redhat.com>, "gdb-patches@sourceware.org"
> > 	<gdb-patches@sourceware.org>
> > Date: Fri, 23 Aug 2013 13:44:58 +0000
> >
> > > > +# check for environment variables to replace certain file
> > > > +properties [ -n "$WIN_EXE_VERSION" ] && version=$WIN_EXE_VERSION
> > > > +[ -n "$WIN_EXE_COMPANY_NAME" ] &&
> > > > +company_name=$WIN_EXE_COMPANY_NAME
> > > > +[ -n "$WIN_EXE_FILE_DESCRIPTION" ] &&
> > > > +file_description=$WIN_EXE_FILE_DESCRIPTION
> > > > +[ -n "$WIN_EXE_PRODUCT_NAME" ] &&
> > > > +product_name=$WIN_EXE_PRODUCT_NAME
> > > > +[ -n "$WIN_EXE_INTERNAL_NAME" ] &&
> > > > +internal_name=$WIN_EXE_INTERNAL_NAME
> > > > +[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] &&
> > > > +original_filename=$WIN_EXE_ORIGINAL_FILENAME
> > > > +[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT [ -n
> > > > +"$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE [ -n
> > > > +"$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED [ -n
> > > > +"$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT
> > >
> > > This looks like unnecessary featurism to me.  Is it really needed,
> > > and if so, in what use cases?
> > Yes, it is needed. Not in here, but for every company which
> changes/adds and re-distributes gdb.
> > I started this feature because our Product Validation does not like
> binaries without legal information.
> > And off course this is different depending on who ships the product.
> > Without a way to change the strings the complete changeset would be
> useless for me.
> 
> You can always modify the source of these attributes, can't you?  It's
> not like you change these strings several times a day, right?
If I have to change/overwrite the sources for this feature just to use the feature, where would be the reason for me to upstream it?  I need parameters/env vars to customize the behavior during build, not changed sources that I need to merge for every branch I have.  The whole purpose of upstreaming a feature is to NOT have custom sources for this feature.

> 
> > > > +#include "afxres.h"
> > >
> > > Is this header really needed?
> > Yes, unfortunately.
> > This header file "implements" the resource language used below in the
> RC file.
> 
> Sorry, I don't understand: this header file in MinGW distribution just
> includes windows.h, defines IDC_STATIC, and that's it.  What do you
> have in it?
As clarified in another feedback - my mistake.  I changed it to 'include "version.h"' in v3.  I also tested it with using Visual Studio on this RC file - still functional.

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

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

[Bunk, Bernd]

> -----Original Message-----
> From: Bunk, Bernd
> Sent: Monday, August 26, 2013 2:25 PM
> To: Eli Zaretskii
> Cc: tromey@redhat.com; palves@redhat.com; gdb-patches@sourceware.org
> Subject: RE: [PATCH v2] Added file properties to windows gdb executable
> for all mingw32 builds.
> 
> > -----Original Message-----
> > From: Eli Zaretskii [mailto:eliz@gnu.org]
> > Sent: Friday, August 23, 2013 4:37 PM
> > To: Bunk, Bernd
> > Cc: tromey@redhat.com; palves@redhat.com; gdb-patches@sourceware.org
> > Subject: Re: [PATCH v2] Added file properties to windows gdb
> > executable for all mingw32 builds.
> >
> > > From: "Bunk, Bernd" <bernd.bunk@intel.com>
> > > CC: "tromey@redhat.com" <tromey@redhat.com>, "palves@redhat.com"
> > > 	<palves@redhat.com>, "gdb-patches@sourceware.org"
> > > 	<gdb-patches@sourceware.org>
> > > Date: Fri, 23 Aug 2013 13:44:58 +0000
> > >
> > > > > +# check for environment variables to replace certain file
> > > > > +properties [ -n "$WIN_EXE_VERSION" ] &&
> > > > > +version=$WIN_EXE_VERSION [ -n "$WIN_EXE_COMPANY_NAME" ] &&
> > > > > +company_name=$WIN_EXE_COMPANY_NAME
> > > > > +[ -n "$WIN_EXE_FILE_DESCRIPTION" ] &&
> > > > > +file_description=$WIN_EXE_FILE_DESCRIPTION
> > > > > +[ -n "$WIN_EXE_PRODUCT_NAME" ] &&
> > > > > +product_name=$WIN_EXE_PRODUCT_NAME
> > > > > +[ -n "$WIN_EXE_INTERNAL_NAME" ] &&
> > > > > +internal_name=$WIN_EXE_INTERNAL_NAME
> > > > > +[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] &&
> > > > > +original_filename=$WIN_EXE_ORIGINAL_FILENAME
> > > > > +[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT [
> > > > > +-n "$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE [ -n
> > > > > +"$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED [ -n
> > > > > +"$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT
> > > >
> > > > This looks like unnecessary featurism to me.  Is it really
> needed,
> > > > and if so, in what use cases?
> > > Yes, it is needed. Not in here, but for every company which
> > changes/adds and re-distributes gdb.
> > > I started this feature because our Product Validation does not like
> > binaries without legal information.
> > > And off course this is different depending on who ships the
> product.
> > > Without a way to change the strings the complete changeset would be
> > useless for me.
> >
> > You can always modify the source of these attributes, can't you?
> It's
> > not like you change these strings several times a day, right?
> If I have to change/overwrite the sources for this feature just to use
> the feature, where would be the reason for me to upstream it?  I need
> parameters/env vars to customize the behavior during build, not changed
> sources that I need to merge for every branch I have.  The whole
> purpose of upstreaming a feature is to NOT have custom sources for this
> feature.
> 
> >
> > > > > +#include "afxres.h"
> > > >
> > > > Is this header really needed?
> > > Yes, unfortunately.
> > > This header file "implements" the resource language used below in
> > > the
> > RC file.
> >
> > Sorry, I don't understand: this header file in MinGW distribution
> just
> > includes windows.h, defines IDC_STATIC, and that's it.  What do you
> > have in it?
> As clarified in another feedback - my mistake.  I changed it to
> 'include "version.h"' in v3.  I also tested it with using Visual Studio
> on this RC file - still functional.
Sorry, spelling error. Of course I meant '#include "windows.h"'
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

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

[Pedro Alves]

On 08/26/2013 01:24 PM, Bunk, Bernd wrote:
>> -----Original Message-----
>> From: Eli Zaretskii [mailto:eliz@gnu.org]
>> Sent: Friday, August 23, 2013 4:37 PM
>> To: Bunk, Bernd
>> Cc: tromey@redhat.com; palves@redhat.com; gdb-patches@sourceware.org
>> Subject: Re: [PATCH v2] Added file properties to windows gdb executable
>> for all mingw32 builds.
>>
>>> From: "Bunk, Bernd" <bernd.bunk@intel.com>
>>> CC: "tromey@redhat.com" <tromey@redhat.com>, "palves@redhat.com"
>>> 	<palves@redhat.com>, "gdb-patches@sourceware.org"
>>> 	<gdb-patches@sourceware.org>
>>> Date: Fri, 23 Aug 2013 13:44:58 +0000
>>>
>>>>> +# check for environment variables to replace certain file
>>>>> +properties [ -n "$WIN_EXE_VERSION" ] && version=$WIN_EXE_VERSION
>>>>> +[ -n "$WIN_EXE_COMPANY_NAME" ] &&
>>>>> +company_name=$WIN_EXE_COMPANY_NAME
>>>>> +[ -n "$WIN_EXE_FILE_DESCRIPTION" ] &&
>>>>> +file_description=$WIN_EXE_FILE_DESCRIPTION
>>>>> +[ -n "$WIN_EXE_PRODUCT_NAME" ] &&
>>>>> +product_name=$WIN_EXE_PRODUCT_NAME
>>>>> +[ -n "$WIN_EXE_INTERNAL_NAME" ] &&
>>>>> +internal_name=$WIN_EXE_INTERNAL_NAME
>>>>> +[ -n "$WIN_EXE_ORIGINAL_FILENAME" ] &&
>>>>> +original_filename=$WIN_EXE_ORIGINAL_FILENAME
>>>>> +[ -n "$WIN_EXE_COPYRIGHT" ] && copyright=$WIN_EXE_COPYRIGHT [ -n
>>>>> +"$WIN_EXE_LICENSE" ] && license=$WIN_EXE_LICENSE [ -n
>>>>> +"$WIN_EXE_CONFIGURED" ] && configured=$WIN_EXE_CONFIGURED [ -n
>>>>> +"$WIN_EXE_SUPPORT" ] && support=$WIN_EXE_SUPPORT
>>>>
>>>> This looks like unnecessary featurism to me.  Is it really needed,
>>>> and if so, in what use cases?
>>> Yes, it is needed. Not in here, but for every company which
>> changes/adds and re-distributes gdb.
>>> I started this feature because our Product Validation does not like
>> binaries without legal information.
>>> And off course this is different depending on who ships the product.
>>> Without a way to change the strings the complete changeset would be
>> useless for me.
>>
>> You can always modify the source of these attributes, can't you?  It's
>> not like you change these strings several times a day, right?
> If I have to change/overwrite the sources for this feature just to use the feature, where would be the reason for me to upstream it?  I need parameters/env vars to customize the behavior during build, not changed sources that I need to merge for every branch I have.  The whole purpose of upstreaming a feature is to NOT have custom sources for this feature.

Environment variables are horrible for build things though.
It's easy to do "make" in one shell that has them set in one way,
then continue work in a different shell that doesn't have them
set in the same way, issue a "make", and end up with a resulting binary
that doesn't have the configuration you really wanted.  Environment
values are also invisible in build logs, which makes use of them for
these kinds of things very frowned upon.  A better solution is one that
avoids these kinds of issues, and that usually means adding a new configure
option instead.  Now, you have a bunch of variables, so it could e.g.,
be something like a comma/colon/whatever- separated list of properties
passed down with --enable-gdb-windows-file-properties=...; or
--enable-gdb-windows-file-properties=/path/to/file, which would
result in create-win_exe_properties.sh reading the values
from /path/to/file; or some other solution along these lines.

Another solution for the whole add-file-properties-to-gdb-binary idea
that crossed my mind is whether one can't just add/rewrite the file
properties in a post-link step (with windres+objcopy perhaps), instead
of having to bake this at gdb build time.  Such a mechanism/tool could
then be used for all binaries/programs in the toolchain, without having
to add resource files all around -- you're going to have to add this
at least for the whole of binutils/gdb/gcc, and probably to more places
too, right?

-- 
Pedro Alves

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

[Pedro Alves]

On 08/26/2013 01:01 PM, Bunk, Bernd wrote:
>> From: Pedro Alves [mailto:palves@redhat.com]
>>>> 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.

Thanks.  Note the icon should probably only be used for gdbtk/insight
builds, not plain command line gdb builds, and should probably stay in
the gdbtk/ dir.  If it's possible to link in more than one rc file,
than there'd be no problem.  But if that's not possible, then we'd
need something more magical...

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

The gdbtk/ directory is part of insight, which (currently) lives in
the same repository but in a different cvs module.  You'll need to
"cvs ... co insight" instead of "cvs ... co gdb" to get it.

-- 
Pedro Alves