Bug 229690 – [git-webkit] Relocate contributors.json

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

contributors.json should be relocated outside of webkitpy, because it's applicable to the entire repository, and used by tooling, not part of tooling.

[Radar WebKit Bug Importer]

<rdar://problem/82552403>

[Jonathan Bedard]

Created attachment 436826 [details]
Patch

[Jonathan Bedard]

Pull-request: https://github.com/WebKit/WebKit/pull/6

[Ryosuke Niwa]

Can we split off the inactive contributors from the top-level JSON while we're at it?

[Ryosuke Niwa]

I also wonder if yaml would be a better fit for this file anyway. JSON is just so verbose!

[Alexey Proskuryakov]

JSON is easier to parse in JavaScript, so probably not (bugs.webkit.org does this).

Not quite sure how to best identify inactive contributors programmatically, but yes, would be nice to have fewer autocomplete suggestions in CC lists.

[Aakash Jain]

We can also consider changing the formatting to make it compact, similar to https://commits.webkit.org/240315@main (r280725)

[Jonathan Bedard]

(In reply to Aakash Jain from comment #7)
> We can also consider changing the formatting to make it compact, similar to
> https://commits.webkit.org/240315@main (r280725)

Compacting contributors.json is a bit more difficult than you might think, because we have a script that re-writes contributors.json with pretty-print.

[Jonathan Bedard]

Created attachment 436901 [details]
Part 1

[Jonathan Bedard]

Created attachment 436904 [details]
Part 2

[Jonathan Bedard]

Created attachment 436907 [details]
Part 2

[Jonathan Bedard]

Created attachment 436908 [details]
Part 3

[Jonathan Bedard]

Created attachment 436912 [details]
Part 4

[Jonathan Bedard]

Created attachment 436914 [details]
Part 3

[Jonathan Bedard]

Created attachment 436921 [details]
Part 4

[Jonathan Bedard]

Created attachment 436925 [details]
Part 5

[Aakash Jain]

Comment on attachment 436901 [details]
Part 1

View in context: https://bugs.webkit.org/attachment.cgi?id=436901&action=review

> ChangeLog:9
> +        * metadata/contributors.json: Copied from Tools/Scripts/webkitpy/common/config/contributors.json, made into list.

As I mentioned on slack, I don't think this is a good idea to modify the file and move it in single commit. As a reviewer, it's extremely hard for me to verify whether there were any mistakes while copying and editing this large file. 

There should be one commit which just move/copy the file without any changes in contributors.json at all (and required changes in other files), so that there isn't need to review the contents of contributors.json. There should be separate commit to change the contents of contributors.json

[Jonathan Bedard]

Comment on attachment 436901 [details]
Part 1

View in context: https://bugs.webkit.org/attachment.cgi?id=436901&action=review

>> ChangeLog:9
>> +        * metadata/contributors.json: Copied from Tools/Scripts/webkitpy/common/config/contributors.json, made into list.
> 
> As I mentioned on slack, I don't think this is a good idea to modify the file and move it in single commit. As a reviewer, it's extremely hard for me to verify whether there were any mistakes while copying and editing this large file. 
> 
> There should be one commit which just move/copy the file without any changes in contributors.json at all (and required changes in other files), so that there isn't need to review the contents of contributors.json. There should be separate commit to change the contents of contributors.json

I understand the sentiment (although the file was generated by validate-committer-lists, as happens when a user modifies contributors.json normally, not manually edited), but the mechanics of editing a file like contributors.json, which is accessed by services that aren't version locked, means that we need a way to determine if we're looking at a new or old version of contributors.json. The way that webkit.org, bugs.webkit.org, ewe-build.webkit.org and webkit-bot all access contributors.json is directly from it's tip-of-tree source via either svn.webkit.org or GitHub. If we don't tie the format change of contributors.json to the rename of the file, these services can't tell if they're dealing with the new file format, or the old one

[Jonathan Bedard]

Aakash and I talked about this via slack, our rough plan is this:
1. Copy existing contributors.json
2. Refactor contributors.json
3-6. Adopt new contributors.json in services

[Jonathan Bedard]

Created attachment 436953 [details]
Patch

[Aakash Jain]

Comment on attachment 436953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436953&action=review

> ChangeLog:9
> +        * metadata/contributors.json: Copied from Tools/Scripts/webkitpy/common/config/contributors.json.

was it copied through svn cp?

[Jonathan Bedard]

Created attachment 436958 [details]
Patch

[Jonathan Bedard]

Comment on attachment 436958 [details]
Patch

Sitting on this change until tomorrow

[EWS]

Committed r281850 (241182@main): <https://commits.webkit.org/241182@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436958 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 437031 [details]
Patch

[Aakash Jain]

Since multiple different improvements are being made, I feel like the improvements should be done in separate bug(s) (with an explanation for that change). Making improvement to contributors.json with a title 'Relocate contributors.json' doesn't feel right.

[Jonathan Bedard]

(In reply to Aakash Jain from comment #27)
> Since multiple different improvements are being made, I feel like the
> improvements should be done in separate bug(s) (with an explanation for that
> change). Making improvement to contributors.json with a title 'Relocate
> contributors.json' doesn't feel right.

I think this goes back to what we were discussing yesterday: the conversion from a dictionary to a list is part of the relocation when it comes to services. This is why I made a pull-request (https://github.com/WebKit/WebKit/pull/6) as well, so you can see why the changes to services need to combine the dictionary -> list change.

If we try and separate the refactor of contributors.json, this is going to be nonsense. We have a sequence of changes which depend on one another, a patch series lets us clearly communicate that dependency chain.

Comment on attachment 437031 [details]
Patch

r=me

[EWS]

Committed r281883 (241213@main): <https://commits.webkit.org/241213@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437031 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 437078 [details]
Part 3

[Jonathan Bedard]

Created attachment 437079 [details]
Part 4

[Jonathan Bedard]

Created attachment 437081 [details]
Part 5

[Jonathan Bedard]

Created attachment 437082 [details]
Patch

[Jonathan Bedard]

Created attachment 437085 [details]
Part 5

[Jonathan Bedard]

Created attachment 437086 [details]
Part 6

[Jonathan Bedard]

Created attachment 437087 [details]
Part 7

[Jonathan Bedard]

Created attachment 437157 [details]
Part 3

[Aakash Jain]

Comment on attachment 437157 [details]
Part 3

rs=me

[Jonathan Bedard]

Created attachment 437158 [details]
Part 4

[Jonathan Bedard]

Created attachment 437159 [details]
Part 5

[Aakash Jain]

Comment on attachment 437158 [details]
Part 4

View in context: https://bugs.webkit.org/attachment.cgi?id=437158&action=review

rs=me with a comment

> Websites/bugs.webkit.org/committers-autocomplete.js:27
> +    var COMMITTERS_URL = 'https://svn.webkit.org/repository/webkit/trunk/metadata/contributors.json';

we should start reading it from github (like ews). from https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json

[Jonathan Bedard]

(In reply to Aakash Jain from comment #43)
> Comment on attachment 437158 [details]
> Part 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437158&action=review
> 
> rs=me with a comment
> 
> > Websites/bugs.webkit.org/committers-autocomplete.js:27
> > +    var COMMITTERS_URL = 'https://svn.webkit.org/repository/webkit/trunk/metadata/contributors.json';
> 
> we should start reading it from github (like ews). from
> https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.
> json

Agreed, but that seemed like something for a different change, this change is already more than we want it to be, I think

[Aakash Jain]

Comment on attachment 437159 [details]
Part 5

View in context: https://bugs.webkit.org/attachment.cgi?id=437159&action=review

> Tools/CISupport/ews-build/steps.py:907
> +        repo_root = os.path.dirname(os.path.dirname(os.path.dirname(cwd)))

It's fine for now, but this method of determining repo root based on location of current file seems fragile, would be good to calculate it in some other reliable method.

[EWS]

Committed r281932 (241241@main): <https://commits.webkit.org/241241@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437157 [details].

[Jonathan Bedard]

Still a few more changes to land.

[EWS]

Committed r281936 (241245@main): <https://commits.webkit.org/241245@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437158 [details].

[Jonathan Bedard]

Still more changes to land.

[EWS]

Committed r281938 (241247@main): <https://commits.webkit.org/241247@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437159 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 437192 [details]
Part 6

[Yusuke Suzuki]

Comment on attachment 437192 [details]
Part 6

r=me Please change the running webkitbot code in the server.

[EWS]

Committed r281956 (241263@main): <https://commits.webkit.org/241263@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437192 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 437222 [details]
Part 7

[Jonathan Bedard]

Comment on attachment 437222 [details]
Part 7

Last one!

Comment on attachment 437157 [details]
Part 3

r=me

Comment on attachment 437222 [details]
Part 7

r=me

[Jonathan Bedard]

Created attachment 440546 [details]
Patch for landing

[EWS]

Committed r283750 (242672@main): <https://commits.webkit.org/242672@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440546 [details].