From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11972 invoked by alias); 26 Aug 2013 12:02:04 -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 11963 invoked by uid 89); 26 Aug 2013 12:02:03 -0000 Received: from mga03.intel.com (HELO mga03.intel.com) (143.182.124.21) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Aug 2013 12:02:03 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RDNS_NONE autolearn=no version=3.3.2 X-HELO: mga03.intel.com Received: from azsmga001.ch.intel.com ([10.2.17.19]) by azsmga101.ch.intel.com with ESMTP; 26 Aug 2013 05:01:58 -0700 X-ExtLoop1: 1 Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by azsmga001.ch.intel.com with ESMTP; 26 Aug 2013 05:01:57 -0700 Received: from irsmsx106.ger.corp.intel.com ([169.254.8.233]) by IRSMSX103.ger.corp.intel.com ([169.254.3.49]) with mapi id 14.03.0123.003; Mon, 26 Aug 2013 13:01:29 +0100 From: "Bunk, Bernd" To: Pedro Alves 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. Date: Mon, 26 Aug 2013 12:02:00 -0000 Message-ID: References: <1377161766-8318-1-git-send-email-bernd.bunk@intel.com> <8361uxkbi9.fsf@gnu.org> <52166564.90605@redhat.com> <521773E9.1060704@redhat.com> In-Reply-To: <521773E9.1060704@redhat.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2013-08/txt/msg00733.txt.bz2 > -----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. >=20 > 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. >=20 > 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? :-) >=20 > 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. B= ut tbh I cannot even find the sources (icon or gdbtk folder). Someone chec= ked in build rules for something that is not on the same branch (upstream/m= aster). I am new to this gdb coding, but I would never do something like t= his... >=20 > > > >> > >> 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] >=20 > I don't see any comment formatting problem in create-version.sh. >=20 > (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 th= is in v3. >=20 > > > >> > >>> + 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=3D"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. > > >=20 > I mean this section: >=20 > http://sourceware.org/gdb/current/onlinedocs/gdbint/Start-of-New-Year- > Procedure.html#Start-of-New-Year-Procedure >=20 > The internals manual has just recently all been dumped into the wiki: >=20 > http://sourceware.org/gdb/wiki/Internals%20Start-of-New-Year- > Procedure >=20 > See "A new strategy for internals documentation" thread in gdb@. >=20 Thanks for these links. I will check this out once the diff has been upstr= eamed 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] >=20 > 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 th= e header file now generates it in the build folder (instead of source folde= r), 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. >=20 > -- > Pedro Alves >=20 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