Erik, Looks good for me!
-Dmitry On 2015-11-17 12:47, Erik Joelsson wrote: > Hello, > > I realized that there already was a mechanism for controlling the > inclusion of SA from configure. Unfortunately, this variable is not > picked up by any makefile currently. I changed the explicit checks for > aix-ppc and zero to instead use the variable INCLUDE_SA which configure > sets up based on at least these conditions. > > I also removed another lingering special case instance of the > jdk.hotspot.agent module in Main.gmk. > > New webrev: http://cr.openjdk.java.net/~erikj/8142336/webrev.03/ > > /Erik > > On 2015-11-13 11:39, Erik Joelsson wrote: >> Widening the distribution in the hopes of finding another reviewer. >> >> I've fixed the formatting in hotspot/make/bsd/makefiles/defs.make >> locally. Merge tool error. >> >> /Erik >> >> On 2015-11-11 17:43, Magnus Ihse Bursie wrote: >>> On 2015-11-11 10:31, Erik Joelsson wrote: >>>> New webrev: http://cr.openjdk.java.net/~erikj/8142336/webrev.02/ >>>> >>>> Fixed the issues listed below. Reverted the faulty attempt at fixing >>>> a warning. Did a more thorough attempt at clearing out all >>>> references to SA in the old makefiles. >>> >>> Looks great. There was a few lines in >>> hotspot/make/bsd/makefiles/defs.make where you seem to have >>> accidentally broken the indentation. Apart from this it looks good. >>> If you just restore the indentation I'm okay (you don't need to >>> respin the webrev for that). >>> >>> /Magnus >>> >>>> >>>> /Erik >>>> >>>> On 2015-11-10 14:49, Magnus Ihse Bursie wrote: >>>>> On 2015-11-10 11:39, Magnus Ihse Bursie wrote: >>>>>> On 2015-11-09 19:33, Erik Joelsson wrote: >>>>>>> Hello, >>>>>>> >>>>>>> As a stepping stone in the hotspot makefile conversion, I have >>>>>>> broken out the serviceability agent separately and converted it >>>>>>> into proper modular build-infra makefiles. Doing this conversion >>>>>>> separately has some value on its own by reducing the special >>>>>>> cases currently needed for building the jdk.hotspot.agent module. >>>>>>> >>>>>>> The current SA java build compiles with the boot jdk javac with >>>>>>> -source/-target JDK N-1. The proposed change instead builds SA >>>>>>> with the interim-langtools javac for JDK N, like all the rest of >>>>>>> the JDK classes. >>>>>>> >>>>>>> There is already a bug filed for reorganizing the source of the >>>>>>> SA agent to conform to the Jigsaw style modular source layout: >>>>>>> JDK-8067194, so I have left the source in its current location. >>>>>>> >>>>>>> The native compilation and linking has been changed to base the >>>>>>> flags used on what configure sets up for the other JDK libraries. >>>>>>> This has caused some changes in flag usage. From what I can tell, >>>>>>> nothing important is different however. I have run the relevant >>>>>>> jtreg tests on all OSes to verify that it still works. Some of >>>>>>> the differences include: >>>>>>> >>>>>>> * Linux: "-Xlinker z -Xlinker defs" was added to LDFLAGS, which >>>>>>> causes link failure unless "-ldl" was also added to LIBS. >>>>>>> * Solaris: More warnings activated through "+w" caused need for >>>>>>> disabling some warnings. I fixed one warning instance which was >>>>>>> trivial (int->size_t), but couldn't figure out the rest. I will >>>>>>> file a followup bug for fixing those if this patch is accepted. >>>>>>> >>>>>>> I tried to mimic the current behavior of excluding SA on >>>>>>> linux-ppc and zero that Volker added a while back. Now it's >>>>>>> excluded on the module level instead so that jdk.hotspot.agent >>>>>>> isn't even built in that case. It would be good if this could be >>>>>>> tested. >>>>>>> >>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8142336 >>>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8142336/webrev.01/ >>>>>> >>>>>> A few remarks: >>>>>> >>>>>> * Could you please document the new DISABLED_WARNINGS_CXX and >>>>>> DISABLED_WARNINGS_C in the function header? >>>>>> >>>>>> * I believe the use of {} here was to signify a set. When only >>>>>> jsig remains, it just looks strange: >>>>>> -# SYMFLAG is used by {jsig,saproc}.make >>>>>> +# SYMFLAG is used by {jsig}.make >>>>>> >>>>>> * The logic of setting up "--hash-style=both" is already done in >>>>>> configure for LDFLAGS_JDKLIB, so you do not need to repeat it, if >>>>>> you include the LDFLAGS_JDKLIB variable. >>>>> >>>>> Also, SA_WINDOWS_LDFLAGS is read but never defined. >>>>> >>>>> /Magnus >>>> >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the source code.
