RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (build system)

Thu May 7 07:14:07 UTC 2020

On 2020-05-07 00:52, Mikael Vidstedt wrote:
>
> Magnus, thank you so much for the thorough review!! Will send out a 
> new webrev in a bit, meanwhile some comments inline..
>
>> On May 6, 2020, at 4:04 AM, Magnus Ihse Bursie 
>> <magnus.ihse.bursie at oracle.com 
>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>>
>> Hi Mikael,
>>
>> On 2020-05-04 07:12, Mikael Vidstedt wrote:
>>> Please review this change which implements part of JEP 381:
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>> webrev: 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/
>>
>> It's a massive change. So you're getting a massive review from me. :-)
>>
>> In the following comments, the line numbers refer to the old files. 
>> (I realize now that it might have been more convenient for you to 
>> have the new numbers. I'm sorry I did not think of it before.)
>
> Not a problem!
>
>>
>> ---
>>
>> First of all, I notice that you have not updated any copyright years. 
>> Do you plan to do that by a script before pushing?
>
> Correct - whether I’ll end up using a script or not remains to be 
> seen, but I’ll fix the copyright years when the reviews/changes have 
> settled down a bit. I chose not to change all the copyright years 
> since that would distract from the “actual” changes, and also pretty 
> much guarantee conflicts when I pull in the latest changes.. It’s 
> challenging enough as it is :)
>
>> ---
>>
>> Several variables are not needed anymore and can be removed. These 
>> include:
>>
>> * TAR_CREATE_EXTRA_PARAM. Remove from Bundles.gmk, 
>> autoconf/basic_tools.m4 and autoconf/spec.gmk.in.
>>
>> * ELFEDIT. Remove from autoconf/spec.gmk.in.
>>
>> * STLPORT_LIB. Remove from autoconf/spec.gmk.in.
>>
>> * SA_LDFLAGS. Remove from modules/jdk.hotspot.agent/Lib.gmk.
>>
>> * GLOBAL_LIBS (as has been noted by Kim as well). Remove from 
>> common/NativeCompilation.gmk, autoconf/spec.gmk.in and 
>> autoconf/libraries.m4.
>>
>> * LDFLAGS_WARNINGS_ARE_ERRORS was (unfortunately!) only supported on 
>> solstudio. No reason to keep it anymore. Remove from 
>> common/NativeCompilation.gmk, autoconf/flags-ldflags.m4 and 
>> autoconf/spec.gmk.in.
>
> Fixed all of the above.
>
>> * GNM is not needed anymore, but there is certainly a need for 
>> related cleanup (we do not need the if statement, but could use 
>> "UTIL_CHECK_TOOLS(NM, nm gcc-nm)" for all platforms). I understand if 
>> you do not want to mess around with it, let me know and I'll file a 
>> separate follow-up bug.
>
> Would definitely appreciate if you could file a follow-up, thank you!
Ok, will do.
>
>> ---
>>
>> In some places we used to have a variable that was passed into 
>> Setup*Compilation, but the variable was removed. However, the 
>> argument to the function was left in place. (Make do not warn for 
>> unknown variables, unfortunately.)
>>
>> lib/CompileJvm.gmk:    EXTRA_OBJECT_FILES := 
>> $(DTRACE_EXTRA_OBJECT_FILES), \
>>
>> Awt2dLibraries.gmk:    ASFLAGS := $(LIBAWT_ASFLAGS), \
>
> Fixed.
>
>> Also, the documentation of SetupNativeCompilation has not been 
>> updated to match the implementation. Please remove the the "REORDER" 
>> line there as well.
>
> I’ve removed REORDER completely now. For completeness: In webrev.00 I 
> (only) removed C_FLAG_REORDER, and didn’t touch the REORDER argument 
> to SetupNativeCompilation. As you noted though, the REORDER 
> functionality is not actually used anywhere - that is true even in 
> mainline today. Turns out the last uses of the REORDER functionality 
> disappeared with https://bugs.openjdk.java.net/browse/JDK-8200178 / 
> http://hg.openjdk.java.net/jdk/jdk/rev/396ea30afbd5 authored by … 
> would you look at that - “ihse”! Wonder who that is? ;)
Ah, I see. I missed the fact that you removed C_FLAG_REORDER and not 
REORDER. Thank you for cleaning up after me anyway. :-)
>
>> ---
>>
>> For dtrace, I want to make sure that the removal is complete (cleanse 
>> it with fire, please! :-)).
>>
>> The build tool generateJvmOffsets is not used anymore, so 
>> generateJvmOffsets.cpp can be removed (actually, that means the 
>> entire hotspot/src/native/).
>>
>> I assume you remove src/java.base/solaris/native/libjvm_dtrace and 
>> libjvm_db in the core-libs patch, right?
>>
>> I also want to make sure that you actually remove hotspot_jni.d, 
>> hotspot.d and hs_private.d from src/hotspot/os/posix/dtrace, and 
>> src/hotspot/os/solaris/dtrace/jhelper.d. (Done in the hotspot patch, 
>> I hope.)
>
> I will not remove all traces of dtrace (no pun intended) as part of 
> this JEP, but (separate from this JEP) we should certainly discuss and 
> decide how to move forward with dtrace on the other platforms. The 
> stuff under src/java.base/solaris is indeed removed as part of the 
> core-libs patch, and the src/hotspot/os/solaris stuff is removed as 
> part of the hotspot patch, but AFAICT the src/hotspot/os/posix/dtrace 
> files still do get picked up by the logic in GensrcDtrace.gmk so I’m 
> going to leave those files untouched. Let me know if I’m missing 
> something.
Ok, I probably came across as too aggressive towards dtrace here. I do 
not mean that I want all of dtrace removed. The parts that are 
cross-platform are quite sane, at least from a build perspective. It's 
all the messy meddling things that dtrace for solaris did that I dislike.

I double-checked now, and you are correct that the 
src/hotspot/os/posix/dtrace files are still needed. I don't know how I 
managed to believe they were not used before. (The power of wishful 
thinking, perhaps.)

/Magnus

>
>> ----
>>
>> More cleanup in lib-freetype.m4:
>>
>> On line 95:
>>   # will report an error. On other platforms (Linux), they will
>>
>> Remove the "(Linux)"; this applies to AIX as well so this 
>> specification is both unnecessary and incorrect. (The best kind of 
>> wrong! :-))
>
> Fixed. :)
>
>> On line 170 and forwards:
>>               # On solaris, pkg_check adds -lz to freetype libs, 
>> which isn't
>>               # necessary for us.
>>               FREETYPE_LIBS=`$ECHO $FREETYPE_LIBS | $SED 's/-lz//g'`
>> Remove these lines, they are not needed anymore.
>
> Fixed (I had that on my follow-up list because I wasn’t 100% sure if 
> it was in fact Solaris-specific).
>
>> ---
>>
>> Some changes I believe you need to revert to keep Zero working on 
>> linux-sparc:
>>
>> In flags.m4, line 268:
>>    test "x$OPENJDK_TARGET_CPU_ARCH" = xsparc ||
>>
>> In jvm-features.m4, line 324:
>>         test "x$OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU" = 
>> "xlinux-sparcv9"; then
>> and line 451:
>>           if test "x$OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU" = 
>> "xlinux-sparcv9"; then
>> JVM_FEATURES_PLATFORM_FILTER="$JVM_FEATURES_PLATFORM_FILTER jfr"
>>
>> In platform.m4, lines 327-330:
>>       elif test "x$OPENJDK_TARGET_CPU_ARCH" = "xsparc"; then
>>         OPENJDK_TARGET_CPU=sparc
>>         AC_MSG_ERROR([Reduced build (--with-target-bits=32) is only 
>> supported on x86_64 and sparcv9])
>> and all the changes between lines 456-487.
>>
>> In CompileJvm.gmk, line 63:
>> else ifeq ($(call isTargetCpu, sparcv9), true)
>>   OPENJDK_TARGET_CPU_VM_VERSION := sparc
>>
>> JvmFeatures.gmk, line 53:
>> This is explicitly done in a check for zero! It needs to be kept. 
>> This also implies that at least the file 
>> src/hotspot/cpu/sparc/memset_with_concurrent_readers_sparc.cpp will 
>> be needed to keep for zero, as well.
>
> I’m counting on somebody (hello Adrian! :)) to provide the list of 
> changes which would have to be reverted/preserved to support zero, 
> otherwise we’ll just be guessing..
>
>> ---
>>
>> In RunTestPrebuilt.gmk, line 191:
>> # ... all others use uname -m
>> This comment is not relevant anymore.
>
> Fixed.
>
>> ---
>>
>> And finally, I want to draw attention to this gold nugget in 
>> CompileDemos.gmk:
>>
>> ifeq ($(call isTargetOs, solaris), true)
>>   TARGETS += $(patsubst $(DEMO_SHARE_SRC)/nbproject/%, \
>>     $(SUPPORT_OUTPUTDIR)/demos/image/nbproject/%, \
>>     $(call FindFiles, $(DEMO_SHARE_SRC)/nbproject))
>> else
>>   TARGETS += $(patsubst $(DEMO_SHARE_SRC)/nbproject/%, \
>>     $(SUPPORT_OUTPUTDIR)/demos/image/nbproject/%, \
>>     $(call FindFiles, $(DEMO_SHARE_SRC)/nbproject))
>> endif
>>
>> LOL! :-D Good thing you cleared this up. :)
>
> I remember staring at that one for a really long time, not seeing what 
> was actually different and not understanding what that meant for my 
> change. Had to go back in the mercurial history to double-check my 
> (in)sanity. :)
>
> Cheers,
> Mikael
>
>>
>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>>>
>>>
>>> Note: When reviewing this, please be aware that this exercise was 
>>> *extremely* mind-numbing, so I appreciate your help reviewing all 
>>> the individual changes carefully. You may want to get that coffee 
>>> cup filled up (or whatever keeps you awake)!
>>>
>>>
>>> Background:
>>>
>>> Because of the size of the total patch and wide range of areas 
>>> touched, this patch is one out of in total six partial patches which 
>>> together make up the necessary changes to remove the Solaris and 
>>> SPARC ports. The other patches are being sent out for review to 
>>> mailing lists appropriate for the respective areas the touch. An 
>>> email will be sent to jdk-dev summarizing all the patches/reviews. 
>>> To be clear: this patch is *not* in itself complete and stand-alone 
>>> - all of the (six) patches are needed to form a complete patch. Some 
>>> changes in this patch may look wrong or incomplete unless also 
>>> looking at the corresponding changes in other areas.
>>>
>>> For convenience, I’m including a link below[1] to the full webrev, 
>>> but in case you have comments on changes in other areas, outside of 
>>> the files included in this thread, please provide those comments 
>>> directly in the thread on the appropriate mailing list for that area 
>>> if possible.
>>>
>>> In case it helps, the changes were effectively produced by searching 
>>> for and updating any code mentioning “solaris", “sparc”, 
>>> “solstudio”, “sunos”, etc. More information about the areas impacted 
>>> can be found in the JEP itself.
>>>
>>>
>>> Testing:
>>>
>>> A slightly earlier version of this change successfully passed 
>>> tier1-8, as well as client tier1-2. Additional testing will be done 
>>> after the first round of reviews has been completed.
>>>
>>> Cheers,
>>> Mikael
>>>
>>> [1] 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>>>
>>
>