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

[Erik Helin]

On 8/31/20 4:04 PM, Philip Race wrote:
> 
> On 8/31/20, 6:11 AM, Kevin Rushforth wrote:
>> The minimum number of reviewers will remain at 1. Any "R"eviewer can 
>> raise the minimum usnig the `/reviewers` command, for example:
>>
>> /reviewers 2
>>
>> This will then require 2 reviewers. We use this for JavaFX and it has 
>> worked well. Having certain areas (e.g., 2d, swing, awt) default to 
>> `/reviewers 2` might be a useful RFE for Skara (Erik or Robin can 
>> comment as to whether they thing this is feasible).
> 
> I would hope so, since the github/skara tooling cheerily tells you can 
> go right ahead and integrate
> once you have the minimum number, and folks will take that message at 
> face value,
> and at least 8/10 of client bugs should have two reviewers so it should 
> be the default.
> Relaxing this is exactly the wrong direction once we push directly to 
> the main jdk repo
> and manually setting them all to two is going to get tedious.

To be precise, the bot states:

 > This change now passes all automated pre-integration checks. When the
 > change also fulfills all [project specific requirements
 > (https://github.com/openjdk/jdk/blob/master/CONTRIBUTING.md), type
 > /integrate in a new comment to proceed. After integration, the commit
 > message will be:

As you can see above we only state that the changes passes the 
_automated_ pre-integration checks. Furthermore we also provide a link 
to additional project specific requirements. For the jdk project, the 
file CONTRIBUTING.md contains a link to https://openjdk.java.net/contribute.

We could potentially highlight *automated* a bit more and also more 
strongly encourage the contributor read the contributing guidelines. As 
I stated in my earlier reply, I don't think it will be possible to 
capture all the details of the informal rules that apply for various 
sub-directories in the jdk repository in a program.

Remember that the guideline is that you should make 8 non-trivial 
changes before you get nomiated to become a Committer (a person who is 
*not* a Committer cannot use /integrate). Reviewers therefore have 
plenty time to educate and guide newcomers during those 8 reviews.

Thanks,
Erik

> -phil
>>
>> I can't think of any good way for automated tooling to be able to 
>> recognize that the "right sort" of reviewers with the right expertise 
>> (someone from 2D someone from build, etc) have reviewed it.
> 
> Nor I, but maybe someone on the skara team does.
> 
> -phil.
> 
>>
>> -- Kevin
>>
>>
>> On 8/29/2020 9:34 AM, 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 ?
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> -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.
>>