code review request for initial JDK FDS support (7071907)

Tue Apr 10 19:09:43 UTC 2012

Kelly,

As always, thanks for filling in the history!

I think I'll stick with changing as a little as possible, but
I will add the comments that I mentioned below...

Dan

On 4/10/12 11:39 AM, Kelly O'Hair wrote:
> Keep in mind that Release.gmk has been edited by many many people and I suspect there are dozens
> and dozens of various bugs in utilities, make itself, the systems being used, shell behaviors, etc.
> that some of this make logic could be working around. I have tried to get people to document that
> in the comments when it happens, but not everyone had or has that style.
>
> On EXE_SUFFIX and LIB_SUFFIX, this was a convention created well before I came on the scene.
> The problem with them is that creating make logic that works in a generic way and includes windows
> is very difficult at times, so very often it ends up being "ifdef windows else ..." in which case these
> EXE_SUFFIX and LIB_SUFFIX macros just confuse the logic and serve no purpose.
> It would be my preference to study how these are used and possible abandon them someday.
>
> I may be the source of the find commands that you claim 'work by accident' but I would claim that
> this specific file and maybe the entire makefile system has that characteristic. :^(
> I very vaguely recall that some of the MKS releases had problems with the find command, or it
> may have been some kind of shell problem where MKS sh would parse command lines twice.
> So the 'work by accident' may have been more of a 'finally got a command line to work'.
> But that's just excuses on my part, if it's wrong, it's wrong, and that was my mistake.
>
> Bottom line is that we should not have to FIND where our exe and dll files are, we should have a
> well defined set of places where they can be found, we need to stop running FIND commands over
> everything all the time, it's a horrible waste of our build time and is really slow on windows. :^(
>
> -kto
>
> On Apr 10, 2012, at 8:03 AM, Daniel D. Daugherty wrote:
>
>> On 4/10/12 7:54 AM, keith mcguigan wrote:
>>> Hi Dan,
>>>
>>> I think it looks good.
>> Thanks!
>>
>>
>>> The new form of the 'find' commands in Release.gmk could use a comment, though.  It took me a few minutes to realize that EXE_SUFFIX was empty for linux/solaris -- it looks like there's a redundancy in the logic (both  NOT name *.debuginfo AND name *.exe).  I suppose there is a redundancy, but only for windows; that new clause is needed only for linux/solaris.
>> These two blocks of make logic make me grind my teeth:
>>
>> 869 $(JRE_BIN_LIST):
>> 870         $(RM) $@
>> 871 ifeq ($(PLATFORM), windows)
>> 872         $(FIND) $(JRE_IMAGE_DIR)/bin -type f -name \*.exe \
>> 873            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)">  $@
>> 874 else
>> 875         $(FIND) $(JRE_IMAGE_DIR)/lib -type f -name \*.$(LIB_SUFFIX)>>  $@
>> 876         $(FILE) `$(FIND) $(JRE_IMAGE_DIR)/bin -type f ! -name '*.debuginfo' -name \*$(EXE_SUFFIX)` \
>> 877             | $(EGREP) 'ELF' | $(CUT) -d':' -f1>>  $@
>> 878 endif
>>
>> 1132 # Get list of binary (COFF or Elf) files in the jdk
>> 1133 JDK_BIN_LIST=$(TEMPDIR)/jdk-bin-files.list
>> 1134 $(JDK_BIN_LIST):
>> 1135 ifeq ($(PLATFORM), windows)
>> 1136         $(FIND) $(JDK_IMAGE_DIR)/jre/bin -type f -name \*.exe \
>> 1137            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)">  $@
>> 1138         $(FIND) $(JDK_IMAGE_DIR)/bin -type f -name \*.exe \
>> 1139            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)">>  $@
>> 1140 else
>> 1141         $(RM) $@
>> 1142         $(FIND) $(JDK_IMAGE_DIR)/jre/lib -type f -name \*.$(LIB_SUFFIX)>>  $@
>> 1143         $(FILE) `$(FIND) $(JDK_IMAGE_DIR)/jre/bin -type f ! -name '*.debuginfo' -name \*$(EXE_SUFFIX)` \
>> 1144             | $(EGREP) 'ELF' | $(CUT) -d':' -f1>>  $@
>> 1145         file `$(FIND) $(JDK_IMAGE_DIR)/bin -type f ! -name '*.debuginfo' -name \*$(EXE_SUFFIX)` \
>> 1146             | $(EGREP) 'ELF' | $(CUT) -d':' -f1>>  $@
>> 1147 endif
>>
>>
>> I originally made more changes here, but I backed them out and went
>> with the minimal changes needed to make FDS work.
>>
>> It's strange that the non-Windows code (lines 876, 1143 and 1144) above
>> uses $(EXE_SUFFIX) which is empty on non-Windows, but it doesn't use it
>> on Windows. Line 876 and line 1143 could probably be rewritten to:
>>
>>     $(FILE) `$(FIND) $(JRE_IMAGE_DIR)/bin -type f ! -name '*.debuginfo'` \
>>
>> but I didn't want to field extra questions about why I changed that.
>> I'm going to add a comment above lines 876 and 1143:
>>
>> # The FILE command reports .debuginfo files as "ELF", but we don't want
>> # those files in the JRE_BIN_LIST file. EXE_SUFFIX is empty on non-Windows.
>>
>> Also, these lines work by accident:
>>
>> 872         $(FIND) $(JRE_IMAGE_DIR)/bin -type f -name \*.exe \
>> 873            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)">  $@
>> 1136         $(FIND) $(JDK_IMAGE_DIR)/jre/bin -type f -name \*.exe \
>> 1137            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)">  $@
>> 1138         $(FIND) $(JDK_IMAGE_DIR)/bin -type f -name \*.exe \
>> 1139            -o -name \*.dll | $(EGREP) -v -i "$(MSVCRNN_DLL)">>  $@
>>
>> There need to be '\(' and '\)' to bind the two '-name' options together.
>> Right now the '-type f' binds to the first '-name' which is then or'ed
>> with the second '-name'. A non-file object with '.dll' extension would
>> get on the list...
>>
>> Dan
>>
>>
>>
>>
>>> -- 
>>> - Keith
>>>
>>> Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> Coming soon to a JDK repo near you! Full Debug Symbols!
>>>>
>>>> OK, to just a subset of libraries and programs... on Linux and Solaris...
>>>> If you're a Windows fan, the JDK repo has had Full Debug Symbols support
>>>> since way back in JDK1.4.1... Now we're trying get Linux and Solaris
>>>> caught up...
>>>>
>>>> Runtime Team, we don't have much in the JDK repo, but I tried to cover
>>>> our few libraries and programs. Let me know if I missed anything...
>>>>
>>>> Serviceability Team, all of your demos, libraries and programs are
>>>> covered... for some reason, updating those seemed like reliving old
>>>> times and I didn't think you'd mind... :-)
>>>>
>>>> Here is the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/fds_revamp/7071907-webrev/0-jdk8-jdk/
>>>>
>>>> Thanks, in advance, for any review comments.
>>>>
>>>> Dan
>>>>
>>>> P.S.
>>>> For those of you that are keeping track of all the FDS
>>>> changesets, not everything has hit the various master
>>>> repos yet. As a reminder, FDS has to hit the closed
>>>> install repo first. The open root and jdk repos along
>>>> with the closed deploy repo are in the second wave. And
>>>> the hotspot repo, being more Mercurial than his fellow
>>>> ghosts, will make his appearance in his own good time
>>>> (and via a different set of repos)...
>>>>
>>>> Apologies to Dickens, of course... :-)
>>>>
>>>>
>>>>