Hi Erik, thanks for reviewing my change! Please find my comments inline:
On Thu, Aug 28, 2014 at 5:56 PM, Erik Joelsson <erik.joels...@oracle.com> wrote: > Hello Volker, > > Thanks for cleaning up! > > > On 2014-08-28 17:22, Volker Simonis wrote: >> >> Hi, >> >> could somebody please review the following small changes to fix the >> AIX build after the Modular Source Code change 8054834. I've also done >> a little cleanup which is detailed below: >> >> http://cr.openjdk.java.net/~simonis/webrevs/8056246/ >> https://bugs.openjdk.java.net/browse/JDK-8056246 >> >> make/lib/Awt2dLibraries.gmk >> >> LIBAWT_CFLAGS contains quite some duplicate include parameters which >> can be removed. >> >> >> -I$(JDK_TOPDIR)/src/java.desktop/$(OPENJDK_TARGET_OS_API_DIR)/native/common/sun/awt >> isn't required because the path >> >> $(JDK_TOPDIR)/src/java.desktop/$(OPENJDK_TARGET_OS_API_DIR)/native/common/sun/awt >> is already part of LIBAWT_DIRS and the corresponding include parameter >> is already generated by $(addprefix -I, $(shell find $(LIBAWT_DIRS) >> -type d)) >> $(foreach dir, $(LIBAWT_DIRS), -I$(dir)) isn't required either, >> because $(addprefix -I, $(shell find $(LIBAWT_DIRS) -type d)) not only >> creates include parameters for every sub-directory contained in >> LIBAWT_DIRS, but also for the directories themselves. > > This all looks good. > >> make/rmic/Rmic-java.management.gmk >> >> This change is a little bit hacky, but it's the best I could find for >> the moment. The problem is that find on AIX (but also GNU find before >> version 4.2) are buggy in the sense that they will report an error if >> "-exec \+" is used in cases where the find command won't find any >> files. >> >> Therefore, the following make rule will fail for incremental builds if >> the $(RMIC_GENSRC_DIR) directory won't contain any *.class files any >> more. These class files are deleted during the first invocation of the >> rule. However, the rule will be re-executed for every new build >> because its target depends on the deleted files. >> >> $(RMIC_GENSRC_DIR)/_the.classes.removed: $(RMI_IIOP) $(RMI_SRC) >> $(FIND) $(RMIC_GENSRC_DIR) -name "*.class" $(FIND_DELETE) >> $(TOUCH) $@ >> >> I've therefore added the file _the.classes.removed to the find >> pattern. That way, the find command will always match at least one >> file and the buggy "-exec \+" won't complain. >> >> $(RMIC_GENSRC_DIR)/_the.classes.removed: $(RMI_IIOP) $(RMI_SRC) >> $(FIND) $(RMIC_GENSRC_DIR) \( -name "*.class" -o -name $(@F) \) >> $(FIND_DELETE) >> $(TOUCH) $@ >> >> Notice, that deleting _the.classes.removed is no problem here, because >> the the file will be recreated by the following touch command. > > This looks safe, but it's not clear why it needs to be done so it certainly > warrants a comment. Another approach on this would be to find a different > way of expressing FIND_DELETE on AIX that works better. Perhaps an xargs > construct? FIND_DELETE is defined in basics.m4 if you would want to give it > a shot. I already thought about this possibility, but then again, AIX find/xargs don't support "-print0"/"-O" which is considered a little unsafe (see for example http://stackoverflow.com/questions/896808/find-exec-cmd-vs-xargs). What do you think, change it to xargs on AIX nevertheless or just add a comment to the current version? >> I've build and smoke tested these changes on Linux/x86_64, >> Linux/ppc64, MacOS X, Solaris10/SPARC and AIX. >> >> Thank you and best regards, >> Volker >> >> PS: I would also like to do some further cleanup in >> NetworkingLibraries.gmk in a follow-up change. I think now that we >> have all the corresponding directories in place we could rename >> {solaris,linux,bsd}_close.c to just close.c and move them to >> $(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/native/libnet. We >> could than get rid of the platform dependent LIBNET_EXCLUDE_FILES in >> NetworkingLibraries.gmk. What's your opinion? > > As Alan said, we intend to follow up on these kinds of changes. I would > welcome any help in getting them started. > Great, thanks. I'll start right after this one is pushed:) Regards, Volker > /Erik
