From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28630 invoked by alias); 23 Aug 2013 14:38:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 28583 invoked by uid 89); 23 Aug 2013 14:38:40 -0000 X-Spam-SWARE-Status: No, score=-8.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 23 Aug 2013 14:38:39 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7NEcZAW009991 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 23 Aug 2013 10:38:36 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7NEcXoa022848; Fri, 23 Aug 2013 10:38:34 -0400 Message-ID: <521773E9.1060704@redhat.com> Date: Fri, 23 Aug 2013 14:38:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 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. References: <1377161766-8318-1-git-send-email-bernd.bunk@intel.com> <8361uxkbi9.fsf@gnu.org> <52166564.90605@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg00681.txt.bz2 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