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

Philip Race philip.race at oracle.com
Mon Sep 7 17:32:31 UTC 2020 

[Removing jdk-dev as we are drilling into a skara discussion deeper than 
needs to be distributed to jdk-dev].

On 8/31/20, 7:16 AM, Erik Helin wrote:
> 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.
I think this is something that needs to be configured at a finer grain 
than that in the JDK project, for the same
reason that there is more than one mailing list. JDK is special. And I 
think there is an additonal subtlety which
you raise below.

>
> 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.

I should clarify that we really do mean more like hotspot "1 Reviewer 
and 1 Author".
We rarely require two capital R reviewers, because active ones are in 
very short supply.
So is the hotspot one enforced by this directory scheme in the skara 
tooling, or are you just referring to "culture" ?

If I do "/reviewers 2" I am now supposing that it really means two 
capital R reviewers and won't accept an author as one of them ?
I think we would really want a way to do what hotspot does - at least 
for the reviewers. We don't have the 24 hour rule.

Also am I correct in understanding that a person proposing a fix cannot 
change (ie either increase or decrease)
the number of reviewers unless they themselves are a reviewer ? I would 
have thought that for some fixes
a committer would know up front what is appropriate and should be able 
to change it. Author maybe not, but
a committer I think should have this capability.

>
>> 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...

I know, but also as I wrote, nothing in the mercurial workflow tells you 
it is ready to integrate contrary to those social
norms, where as in the case of skara it does.
The developer's guide is alas way too far removed from the immediacy of 
the message you get from skara to be an effective deterrent.
So I think we are going to get a lot of cases where people don't wait 
who should have waited.
They will believe the tooling indicates process has changed.

At the very least the message about being ready to integrate could 
carry  a warning that some areas require a 2nd
review beyond the base configuration of the project and eg if this is a 
client fix, please wait ..


-phil.

>
> 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.

More information about the skara-dev mailing list