Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Pedro Alves <palves@redhat.com>
To: Eli Zaretskii <eliz@gnu.org>
Cc: Bernd Bunk <bernd.bunk@intel.com>,
	tromey@redhat.com,        gdb-patches@sourceware.org
Subject: Re: [PATCH v2] Added file properties to windows gdb executable for all mingw32 builds.
Date: Thu, 22 Aug 2013 19:24:00 -0000	[thread overview]
Message-ID: <52166564.90605@redhat.com> (raw)
In-Reply-To: <8361uxkbi9.fsf@gnu.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?  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


  reply	other threads:[~2013-08-22 19:24 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-22  8:56 Bernd Bunk
2013-08-22 15:09 ` Eli Zaretskii
2013-08-22 19:24   ` Pedro Alves [this message]
2013-08-22 19:55     ` Eli Zaretskii
2013-08-23 13:06       ` Pedro Alves
2013-08-23 13:27         ` Eli Zaretskii
2013-08-23 13:42           ` Pedro Alves
2013-08-23 14:09             ` Bunk, Bernd
2013-08-23 14:07     ` Bunk, Bernd
2013-08-23 14:38       ` Pedro Alves
2013-08-26 12:02         ` Bunk, Bernd
2013-08-26 15:42           ` Pedro Alves
2013-08-23 13:45   ` Bunk, Bernd
2013-08-23 14:20     ` asmwarrior
2013-08-23 14:56       ` Bunk, Bernd
2013-08-23 14:37     ` Eli Zaretskii
2013-08-26 12:25       ` Bunk, Bernd
2013-08-26 15:24         ` Pedro Alves
2013-08-26 12:34       ` Bunk, Bernd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52166564.90605@redhat.com \
    --to=palves@redhat.com \
    --cc=bernd.bunk@intel.com \
    --cc=eliz@gnu.org \
    --cc=gdb-patches@sourceware.org \
    --cc=tromey@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox