RFR: JDK-8199681 Remove boilerplate code from creating native jtreg tests

[David Holmes]

On 16/03/2018 9:43 PM, Magnus Ihse Bursie wrote:
> On 2018-03-16 03:48, David Holmes wrote:
>> Hi Magnus,
>>
>> Removing boilerplate is good but the exclude mechanism seems rather 
>> awkward compared to the previous include. It's much more obvious when 
>> writing a platform specific native test to include it for that 
>> platform, rather than exclude it for all other platforms. You run the 
>> risk of having excludes spread all over the place, or not knowing 
>> where you should put them. I thought the opt-in approach of just 
>> adding the test directory to the list of native test directories was 
>> simple and worked well. (The need to add the extra link instructions 
>> wasn't good though, :) )
> 
> Previously you had to write like:
> ifeq ($(OPENJDK_TARGET_OS), windows)
> SRC += mytest
> endif
> 
> Now you have to write
> ifneq ($(OPENJDK_TARGET_OS), windows)
> EXCLUDE += mytest
> endif
> 
> Seriously, that's not much harder.

True. It just doesn't look that clean in the actual file.

> But the latter has the extremely important benefit that for all 
> multi-platform tests (which comprise the absolute majority), you don't 
> have to modify the makefiles at all.

True.

>> Also having to exclude on the file level is more awkward than using 
>> the directory level.
> I can of course change that. I chose the file level due to these reasons:
> * We have a requirement of filename uniqueness across all tests (since 
> they compile to object files in a single directory); however we do not 
> have a requirement of directory name uniqueness.

Given we used to include directories it seems natural to me that we now 
exclude directories. Though I suppose files does give more fine grained 
control if we were to need it.

> * Basing this on filenames allows you to single out individual tests 
> that are, for logical reasons, grouped together in a single directory.
> * And it was easier to implement in make scripts. :-)
> 
> As a suggestion, I can add a PATTERN_EXCLUDE. This way it will work 
> about the same as the Setup*Compilation functions, were you can exclude 
> based on file name or pattern matching. Do you want me to do that? I 
> think with the current number of excludes, it's not really worth it, but 
> if it is important to you, I can fix it.

Let's just see how it goes, as you've already pushed it.

Thanks,
David

>>
>>
>> 49 BUILD_HOTSPOT_JTREG_IMAGE_DIR := $(TEST_IMAGE_DIR)/hotspot/jtreg
>>
>> I thought you'd changed something here then noticed that we end up doing:
>>
>>  94     DEST := $(TEST_IMAGE_DIR)/hotspot/jtreg/native, \
>>
>> and BUILD_HOTSPOT_JTREG_IMAGE_DIR is actually unused.
> 
> *duh*
> 
> I have already pushed the patch. I will fix this in a follow-up bug. Let 
> me know if you want to have a directory-based or pattern- based 
> exclusion mechanism in that as well.
> 
> /Magnus
> 
>>
>> Thanks,
>> David
>>
>> On 15/03/2018 11:01 PM, Magnus Ihse Bursie wrote:
>>> There's currently a lot of boilerplate code needed to setup a new 
>>> native jtreg test. This was never the way it was intended to work -- 
>>> the idea was that you basically should just add the file and then 
>>> things should work automatically, unless you had special requirements.
>>>
>>> This patch will make it so once more.
>>>
>>> I have tested this using COMPARE_BUILD. I get some spurious binary 
>>> differences on macosx. I can't really say why, but then again, we 
>>> have never verified the test image by a clean "back-to-back" null 
>>> comparison either, so I'm not worried. Apart from that, it looks green.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199681
>>> WebRev: 
>>> http://cr.openjdk.java.net/~ihse/JDK-8199681-remote-native-test-boilerplate/webrev.01 
>>>
>>>
>>> /Magnus
>