RFR: Data plugin

[Alexandre Iline]

On Wed, 27 Apr 2022 15:30:19 GMT, Alexandre Iline <shurailine at openjdk.org> wrote:

>> plugins/data_coverage/src/openjdk/jcov/data/Env.java line 63:
>> 
>>> 61:         Set<String> keys = System.getProperties().stringPropertyNames();
>>> 62:         keys.stream().filter(k -> k.startsWith(prefix))
>>> 63:                 .forEach(k -> System.clearProperty(k));
>> 
>> It's not good idea to clear/update system properties. Actually much better to use an "application" copy of them and interact with the copy as needed. Or after program shut down updated system properties could be used by an other application?
>
> The way the plugin is designed, its behavior is controlled through the system properties. This is done this way to allow usage of jcov command line, such as Instr, JREInstr, etc, to specify how the code is instrumented, why code, where results are saved, etc. As such, it is _unavoidable_ to have the plugin code to _read_ the system properties. Here are a few examples of system properties which specifies the plugin behavior: Instrument.PLUGIN_CLASS, Instrument.JCOV_TEMPLATE, Collect.COVERAGE_OUT, Collect.COVERAGE_IN, Plugin.METHOD_FILTER, Collect.SERIALIZER
> 
> An additional level is added to the instrumentation to allow, during instrumentation time, to specify properties which will be needed during the  execution time. This is just a convenience which shortens command line needed to run the tests later. Instead of listing the properties on the command line when running the tests, the properties are stored within the instrumented code. This is, coincidently, a convenience which I would be handy for normal, non-plugin, Jcov functionality: to be able, during the instrumentation, specify a custom grabber port, to use an example. I thought, therefore, that the solution which I was suggesting was an elegant one, since the system properties which are set during the execution are exactly the same properties which the plugin is expected to be set as system properties to work.
> 
> I suppose it is possible to change the code of the plugin to try to load properties from a local storage rather than from the system properties. I will have to take a close and a hard look to see if it can be done nicely.

As for reusing the system property by another application after the program shut down, I do not really know what you mean. The properties which are set are the ones which may be set anyway to control the plugin behavior.

>> plugins/data_coverage/src/openjdk/jcov/data/Env.java line 66:
>> 
>>> 64:     }
>>> 65: 
>>> 66:     public static String getStringEnv(String property, String defaultValue) {
>> 
>> defaultValue isn't used and could be omited. 
>> line 70 of Class Collect  
>> 
>> if (!Env.getStringEnv(COVERAGE_IN, "").isEmpty()) {
>> ``` 
>> could be chaged to 
>> 
>> if( Env.getStringEnv(COVERAGE_IN) != null
>> 
>> Also isn't clear why you wraps IOException with UncheckedIOException here.
>
> on line 115:
> } else return defaultValue;

on line 70 of Collect, I am defending against COVERAGE_IN property been set to an empty string as well as not been set at all. The code which you suggested is not equivalent to what I am having.

-------------

PR: https://git.openjdk.java.net/jcov/pull/19