code review request for initial JDK FDS support (7071907)

Tue Apr 10 17:39:21 UTC 2012

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