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
