Looks good to me now.
/Erik
On 2017-03-03 10:39, Magnus Ihse Bursie wrote:
I have an updated webrev.
Here is a differential webrev showing the changes compared to the
previous webrev:
http://cr.openjdk.java.net/~ihse/JDK-8176084-introduce-run-test/webrev.02
And here is a full webrev showing all changes:
http://cr.openjdk.java.net/~ihse/JDK-8176084-introduce-run-test/webrev.03
Replies to specific comments inline.
On 2017-03-02 11:40, Erik Joelsson wrote:
Hello Magnus,
Overall this is excellent work! But I still have some opinions and
questions.
I think the implementation of the caching mechanism is a bit weird
and it will not handle changes to jtreg test groups (or other test
name definition files). I don't think we can expect people to have to
manually clean if they edit such a file. I would very much prefer if
this was done the same way as we calculate modules. This would be
achieved by: 1. including FindTests.gmk from Main.gmk instead of
shell-call. 2. In FindTests.gmk -include $(NAMED_TESTS_INCLUDE). 3.
In FindTests.gmk add all actual source files for test names as
prerequisites to $(NAMED_TESTS_INCLUDE). 4. Modify FindTests.gmk to
only actually do work if the recipe for $(NAMED_TESTS_INCLUDE) is
triggered. This can be achieved by moving the calls to the recipe.
This means the variables you generate are always read from the
generated makefile.
Yeah, I agree, it's a bit corny. It grew out of a non-caching
solution. According to our off-list discussion, I have now removed the
caching mechanism instead. It didn't do much for performance and just
complicated things. Instead I include FindTests.gmk where it is needed.
FindTests.gmk
51: This conditional seems redundant, did you mean to put a wildcard
there?
Yes I did. Thanks.
63: should be 2 spaces
Really? I think it's a single line I had to break into two, similar to
what's on line 66. Do you consider it to be a logical indentation? If
so, I should probably move the closing parenthesis to a separate line,
but that feels overkill. In general, I have not considered simple
foreach:es to be logical construct but one-liners.
MakeBase.gmk
772: an -> a
Fixed.
774: did you really mean = and not :=?
No, not really. :-) Fixed.
Note that when using $(eval) inside a macro body that will also be
evaled, you actually need 4 dollars on all references inside it if
you want to be sure to handle any $ in the variable values correctly.
In this case, if $ is ever a character in one of the parameters to
jtreg/gtest, it's likely going to fail. Then we have the special case
of the foreach local variable, which has to be referenced with the
same amount of dollars as the foreach call, so you can't just put 4
on it. Here is an example of how I figured you can deal with this:
$$(foreach f, $$($1_SRC_FILES), \
$$(eval fd := $$(call DoubleDollar, $$(f))) \
$$(if $$(filter /%, $$(f)), \
$$(eval r := $$$$(patsubst $$$$($1_SRC_BASE_DIR)/%, %,
$$$$(fd))) \
) \
)
Basically for the foreach variable, you need to create a doubledollar
version and use the new one in evals, with 4 dollars, and the
original in all other cases. Yes, this becomes very convoluted.
No sh*t. :-& I've updated the code to match this pattern.
RunTests.gmk
74: I thought Christian made the default for hotspot be min(12,
$(NUM_CORES) / 2)?
I tried to mimic what I could extract from the existing logic.
However, I see now that you say it that I've missed some
concurrency-related hacks in the old hotspot JTreg Makefile. (Both
this and the MaxRAMFraction). Talk about convoluted... :-( I fixed
this now by porting the old behavior verbatim, but I think this area
could need some cleanup in the future.
Also, I noticed that the fix to limit JTreg jobs to 50 had disappeared
somewhere when I worked with handling the hotspot special logic. I
have now restored that.
238: The command is wrapped in ExecuteWithLog, why pipe to yet
another log file?
For consistency, to store the log output in the rest-results
directory, like the other test suites do. Gtest does not have a
built-in way of storing the test output to a log file. I could have
copied the ExecuteWithLog file at the end, but that felt like I
meddled with a private part of the interface. What if we introduce a
way to disable logging in ExecuteWithLog? Then the gtest parsing would
fail.
242,361: shouldn't parse-test have run-test as prerequisite (even if
.NOTPARALLEL is in effect)?
Yes, it should indeed. Fixed.
261: Would be nice with a line of # heading this section too
Okie-dokie.
325: The MaxRAMFraction for at least hotspot was made dynamic on the
concurrency AFAIK
Correctly. I missed that part as well. Fixed.
430: I think it would be better just adding $(TARGETS) as prereq
Better like this?
/Magnus
/Erik
On 2017-03-02 08:42, Magnus Ihse Bursie wrote:
A long-time issue has been a consistent way for developers to
effortlessly run tests on local builds. This patch introduces a new,
alternative "run-test" target, which allows for a smoother developer
experience in running tests. It does not modify or remove any
existing ways of running tests, which are still needed for automated
test systems and old scripts.
Bug: https://bugs.openjdk.java.net/browse/JDK-8176084
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8176084-introduce-run-test/webrev.01
/Magnus
