+1
-phil.
On 05/12/2016 11:02 AM, Tim Bell wrote:
Erik:
Looks good to me as well.
/Tim
On 05/12/16 04:06, Erik Joelsson wrote:
Good point, the quotes are certainly not necessary. Removed and
updated review in place. Verified that it still works.
/Erik
On 2016-05-12 12:49, Vadim Pakhnushev wrote:
Erik,
I wonder why in the Launcher-java.base.gmk first -I flag parameter
is not quoted, but subsequent flags parameters are quoted?
I guess the quotes are unnecessary here...
Vadim
On 12.05.2016 13:22, Erik Joelsson wrote:
When using the RC tool on Windows, we also run CL with the same
arguments to generate dependencies on included files. In some
cases, extra include directories are provided in the RC_FLAGS,
using the flag -i (lower case). This flag works for the RC tool,
but not for CL, which requires -I (upper case). The upper case
version works for RC as well so we should switch to using upper case.
Another problem uncovered by this is that NativeCompilation.gmk
isn't handling this failure well. It calls CL through the
ExecuteWithLog macro. This macro will copy the log to the
failure-logs dir if the command returns non zero. Because of the
above problem, the command returns non zero. NativeCompilation.gmk
ignores this so the build continues. If the build then fails for
any other reason, the failure report will find these failure logs
and fool the user into thinking this was the cause of the build
failure.
I propose to fix this by changing NativeCompilation.gmk to not
ignore the return code of CL and let the output of CL through, but
filtered the same way as when we do normal compilation. Then the
build will actually fail if CL fails here so there is no
discrepancy between failure-logs and the actual failure. I also
removed the piping to a new file since ExecuteWithLog already pipes
output to a file so we can just use that.
To make this work, we must also ensure that no bad arguments reach
CL here. This is done by filtering known bad arguments (currently
only -l0x409 which I chose to filter with +l%, note that this is
lower case l and not upper case i), and of course changing -i to -I
in all places.
Bug: https://bugs.openjdk.java.net/browse/JDK-8156837
Webrev: http://cr.openjdk.java.net/~erikj/8156837/webrev.01/
/Erik
