Request for assistance: Verify and update mailing list filter rules for jdk/jdk in preparation for Skara transition

[Erik Helin]

On 8/29/20 6:34 PM, Philip Race wrote:
> I have some questions which at least tangentially relate to the list 
> mappings which raise
> what to me are tricky problems but maybe they've already been thought 
> through and solved.
> 
> First, what is the minimum number of reviewers required a fix for the 
> JDK project ?

It is 1 Reviewer as configured by .jcheck/conf.

> Second, how can we have that by default adjusted depending on mailing 
> list, with the
> ability to either raise or lower that number according to the judgement 
> of the fixer.
> In the client area 2 reviewers is the big rule, only one has to be a 
> Capital R reviewer,
> but for trivial and urgent fixes we allow just one reviewer.

Ok, so we have to separate between culture and rules. The .jcheck/conf 
file configures the rules, and the rule is 1 Reviewer.

For several directories in the jdk repository various norms have formed 
with regards to the requirements that needs to be met before one can 
push. For example for the src/hotspot directory you need 1 Reviewer and 
1 Author (or above) to review and you should wait 24 hours before 
pushing. For the client directories you state that 2 Reviewers are 
needed (I wasn't aware, I have never contributed to the client code). 
For the make directory you really ought to get a review from either Erik 
J or Magnus. These norms are not mechanically enforced (not today by 
jcheck, nor tomorrow by Skara), instead they are enforced through 
Reviewers educating contributors about the cultural norms.

> Third, if a fix is cross-posted to (say) build-dev and 2d-dev, how do 
> you ensure it
> gets both a build reviewer and a 2D reviewer's approval before pushing ? 
> That
> is after all the point of the cross-posting.

The norm that a change to the Makefiles must be reviewed by a senior 
build engineer (in most cases Erik J or Magnus) is not mechanically 
enforced, as I explained above. The point of cross-posting to build-dev 
is to allow an experienced build engineer to get the chance to look at 
the patch, otherwise the build engineers would have follow all mailing 
lists.

I agree with Kevin that trying to capture all these cultural norms 
automatically is going to be very tricky (if not impossible). Perhaps 
the developers guide could be a good place to store this information?

Remember that today with Mercurial we don't really enforce anything, as 
you can put any Reviewers OpenJDK username on the "Reviewed-by" line and 
then push. A Committer doing this would probably find themselves losing 
their Committer status very quickly though...

Thanks,
Erik

> -phil.
> 
> On 8/27/20, 10:37 AM, Joe Darcy wrote:
>> Hello,
>>
>> The mapping of 
>> changed-file-to-OpenJDK-mailing-list(s)-to-review-the-file can be 
>> non-obvious, both to new contributors and to experienced contributors 
>> working in an unfamiliar area. If an automated mapping gets this 95+% 
>> correct, with the ability for manual tweaking, that strikes me as an 
>> overall improvement over the current situation.
>>
>> The current mapping has various entries that should be changed, but 
>> that is why it is being sent out for review before the Skara 
>> transition :-)
>>
>> As a general comment, I would expect both the mapping and other 
>> aspects of the Skara tooling to get updated in response to feedback 
>> after jdk/jdk moves over.
>>
>> -Joe
>>
>> On 8/27/2020 9:53 AM, Igor Ignatyev wrote:
>>> Hi Robin,
>>>
>>> although I really like the idea of having mapping b/w files and 
>>> groups/components/subcomponents, I agree w/ David that in its 
>>> *current* form it's unworkable. having the mapping in Skara repo is 
>>> impractical, as it introduces additional overhead for maintenance, 
>>> not to speak of possible desynchronization. Thus I suggest moving the 
>>> mapping to jdk/jdk repo and updating Skara tooling accordingly.
>>>
>>> I also have a question regarding the format, why did you decide to 
>>> invent your own format instead of using CODEOWNER-like format which 
>>> fits the needs rather nicely and is used for similar purposes by 
>>> github and gitlab?
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>> On Aug 27, 2020, at 6:26 AM, David Holmes <david.holmes at oracle.com> 
>>>> wrote:
>>>>
>>>> In all seriousness I just don't think this is a reasonable or 
>>>> necessary thing to do. When you create a PR you should specify the 
>>>> mailing lists to be used, as you do today with a RFR. Trying to 
>>>> maintain a file by file mapping is just a huge initial setup cost 
>>>> and a maintenance nightmare. It is not necessary to try and automate 
>>>> this IMO.
>>>>
>>>> I wish this intent had been flagged/discussed some time ago. :(
>>>>
>>>> David
>>>> -----
>>>>
>>>> On 27/08/2020 8:34 pm, Robin Westberg wrote:
>>>>> Hi all,
>>>>> As part of transitioning the jdk/jdk repository to Git with project 
>>>>> Skara, we have created a set of rules that determine which mailing 
>>>>> list(s) should be the default recipient of a review request, 
>>>>> depending on which files are changed. The initial version of these 
>>>>> rules was created by looking at historical commits and matching 
>>>>> them with existing mailing list review threads. This has produced a 
>>>>> reasonable basis, but it can most certainly be made better with 
>>>>> some additional manual curating.
>>>>> Therefore, it would be very helpful if people with good knowledge 
>>>>> of the various subsystems and source files that make up the JDK 
>>>>> would be willing to take a look at these rules, and also suggest 
>>>>> improvements where needed. In addition, lists like [1] would also 
>>>>> be very useful insofar they exist.
>>>>> The current version of these rules is located in a JSON file in the 
>>>>> Skara repository at [2]. In order to check the validity of the 
>>>>> rules, there is also a CLI tool that can be used to apply it to 
>>>>> either a subset of files or existing commits and produce a 
>>>>> suggestion [3] [4]. If you are interested in helping out with 
>>>>> curating these rules, these are the steps to get started:
>>>>> 1. Install the Skara CLI tools: 
>>>>> https://wiki.openjdk.java.net/display/SKARA/CLI+Tools
>>>>> 2. Locate a suitable clone of the jdk/jdk repository (either 
>>>>> Mercurial or Git is fine)
>>>>> 3. Change (cd) to the root of your jdk/jdk repository
>>>>> 3. Run the “debug mlrules” command on your favorite subset of 
>>>>> files, for example like this (use the actual location of jdk.json 
>>>>> on your system):
>>>>> $ git skara debug mlrules -v 
>>>>> ~/git/skara/config/mailinglist/rules/jdk.json src/hotspot/share/jfr/
>>>>> Reading rules file...
>>>>> src/hotspot/share/jfr/dcmd: [hotspot-jfr-dev]
>>>>> src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp: 
>>>>> [hotspot-jfr-dev]
>>>>> src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.hpp: 
>>>>> [hotspot-jfr-dev]
>>>>> src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp: 
>>>>> [hotspot-jfr-dev, serviceability-dev]
>>>>> …
>>>>> Final list suggestion is: [hotspot-jfr-dev, serviceability-dev]
>>>>> The command accepts multiple folder and/or file names to make it 
>>>>> possible to simulate a potential change to a given set of files:
>>>>> $ git skara debug mlrules -v 
>>>>> ../skara/config/mailinglist/rules/jdk.json doc/ide.md 
>>>>> src/hotspot/cpu/x86/x86.ad src/hotspot/os/linux/gc/z/zNUMA_linux.cpp
>>>>> Reading rules file...
>>>>> doc: [build-dev]
>>>>> src/hotspot/cpu: [hotspot-compiler-dev]
>>>>> src/hotspot/os: [hotspot-runtime-dev, hotspot-gc-dev]
>>>>> Combined list suggestion: [build-dev, hotspot-compiler-dev, 
>>>>> hotspot-gc-dev, hotspot-runtime-dev]
>>>>> Final list suggestion is: [build-dev, hotspot-dev]
>>>>> If the suggestions look fine, all is well. If not, you are welcome 
>>>>> to propose a change to the rules, preferably by editing the 
>>>>> jdk.json file [6] and creating a pull request towards the Skara 
>>>>> project as described in [5]. Coincidentally, this is the same way 
>>>>> that future changes to the jdk/jdk repository will be integrated, 
>>>>> so this exercise could also serve as a way of getting started with 
>>>>> Git / Skara!
>>>>> Best regards,
>>>>> Robin
>>>>> [1] https://openjdk.java.net/groups/2d/2dawtfiles.html
>>>>> [2] 
>>>>> https://git.openjdk.java.net/skara/blob/master/config/mailinglist/rules/jdk.json 
>>>>>
>>>>> [3]
>>>>> $ git skara debug mlrules -v 
>>>>> ~/git/skara/config/mailinglist/rules/jdk.json 
>>>>> src/java.desktop/unix/native
>>>>> Reading rules file...
>>>>> src/java.desktop/unix/native/common: [2d-dev]
>>>>> src/java.desktop/unix/native/include: []
>>>>> src/java.desktop/unix/native/libawt: [2d-dev]
>>>>> src/java.desktop/unix/native/libawt_headless: [awt-dev]
>>>>> src/java.desktop/unix/native/libawt_xawt: [awt-dev]
>>>>> src/java.desktop/unix/native/libfontmanager: [2d-dev]
>>>>> src/java.desktop/unix/native/libjawt: [awt-dev]
>>>>> src/java.desktop/unix/native/libsplashscreen: [awt-dev]
>>>>> Combined list suggestion: [2d-dev, awt-dev]
>>>>> Final list suggestion is: [2d-dev, awt-dev]
>>>>> [4]
>>>>> $ git skara debug mlrules -d 30 -v 
>>>>> ~/git/skara/config/mailinglist/rules/jdk.json .
>>>>> ...
>>>>> ✅ [2d-dev, awt-dev, serviceability-dev] c32923e0: 8240487: Cleanup 
>>>>> whitespace in .cc, .hh, .m, and .mm files
>>>>> ❌ [awt-dev] 7f74c7dd: 8212226: SurfaceManager throws "Invalid Image 
>>>>> variant" for MultiResolutionImage (Windows)
>>>>>      Suggested lists: [2d-dev, awt-dev]
>>>>>      Rules matching unmentioned lists [2d-dev]:
>>>>>        src/java.desktop/share/classes/sun/java2d/SunGraphics2D.java 
>>>>> - [2d-dev: ^src/java.desktop/share/classes/sun/java2d/]
>>>>> …
>>>>> [5] https://wiki.openjdk.java.net/display/SKARA/Skara#Skara-Workflow
>>>>> [6]
>>>>> The rules are made up of sets of regular expressions for the 
>>>>> various mailing lists that are used for reviewing changes going 
>>>>> into the JDK. If any rule matches, that mailing list will get a 
>>>>> copy of the review request email. For directories containing files 
>>>>> that belong to different subsystems, it’s usually a good idea to 
>>>>> write the rules in a complementary fashion if possible, so that 
>>>>> anything not explicitly mentioned gets a reasonable default. As an 
>>>>> example, see these rules for a subset of awt / 2d / i18n files:
>>>>> “awt-dev”: 
>>>>> "src/java.desktop/share/classes/sun/awt/(?!font|sunhints|color/|font/|geom/|im/|image/|print/)” 
>>>>>
>>>>> “2d-dev”: 
>>>>> "src/java.desktop/share/classes/sun/awt/(font|sunhints|color/|font/|geom/|image/|print/)" 
>>>>>
>>>>> “I18n-dev”: "src/java.desktop/share/classes/sun/awt/im/“
>>>>> In this example, anything not explicitly indicated as belonging to 
>>>>> either 2d-dev or i18-dev will be matched by awt-dev.