[webkit-reviews] review denied: [Bug 177225] Update syncing script to be able to build binary for commit set with owned commits. : [Attachment 321309] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 177225: Update syncing script to be able to build binary for commit set
with owned commits.
https://bugs.webkit.org/show_bug.cgi?id=177225

Attachment 321309: Patch

https://bugs.webkit.org/attachment.cgi?id=321309&action=review




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 321309
  --> https://bugs.webkit.org/attachment.cgi?id=321309
Patch

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

> Websites/perf.webkit.org/ChangeLog:10
> +	   'requiresBuild' should be true whenever commit set item, the
repository of which is in the list specified by 'requireBuild', has
'requiresBuild' set to true.

This line is too long. Please wrap it earlier.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:55
> +	       if (item.commitOwner)
> +		   this._ownedRepositories.push(commit.repository());

I don't think we need to build this map separately.
We can just traverse through keys of _repositoryToCommitOwnerMap.
In fact, I'd suggest we don't store a value into _repositoryToCommitOwnerMap
when item.commitOwner is null/undefined.

In general, it's a bad idea to have multiple data structures that need to be
kept in sync.
That can lead to bugs if we ever mutate CommitSet for example.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:194
> +	   if (ownerRevision)
> +	       this._ownedRepositories.push(repository);

Ditto. We should be able to filter keys of _revisionListByRepository by its
value not having ownerRevision.

> Websites/perf.webkit.org/public/v3/models/triggerable.js:68
>	   // FIXME: Add a check for patch.

Looks like this FIXME is obsolete. Remove?

> Websites/perf.webkit.org/public/v3/models/triggerable.js:70
> +	   const ownedRepositorySet = new Set(commitSet.ownedRepositories());
> +	   const commitSetRepositories =
Repository.sortByNamePreferringOnesWithURL(commitSet.repositories().filter((rep
ository) => { return !ownedRepositorySet.has(repository); }));

Instead of all this work here, we should simply add a method which returns a
sorted list of repositories that don't have an owner.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:263
> +	       case 'pairs':

This is such a vague/generic name. It doesn't tell me anything about what kind
of pair this is.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:267
> +		   value =
JSON.stringify(ownedRepositories.map((ownedRepository) => {

So this is not a pair at all. It's an array of dictionaries!
It's a shortsighted design to put all owned repositories into one JSON.
I think we should just get a list of all owned revisions for a given owner
repository instead.
After making that change, this option can be called "ownedRevisions" so that we
can specify:
e.g. {"ownedRevisions": "iOS"}.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:273
> +			   'owner-revision':
commitSet.ownerRevisionForRepository(ownedRepository),
> +			   'owner-repository':
commitSet.ownerCommitForRepository(ownedRepository).repository().name()

We should use camelCase instead.
If we've made the change to only specify the owned revisions for a given owner
repository,
then ownerRepository can be removed.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:279
> +	       case "requiresBuild":
> +		   value =
value.repositoriesToCheck.map(commitSet.requiresBuildForRepository.bind(commitS
et)).some((requiresBuild) => requiresBuild);
> +		   break;

This is wrong. When WebKit doesn't need to be built, the property must not be
set.
Also, this needs to be folded into 'conditional' type below. r- because of
this.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:471
> +		   case 'pairs':

We shouldn't be using such a vague term for the option name.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:474
> +		       return {type, repositoriesToCheck:
value.map(resolveRepository)};

I don't think it makes sense to call this option "requiresBuild" since what the
user of this feature is doing is specifying a list of repositories, if set,
should trigger a build.
How about something like "ifRepositoriesSet".
Also, this should be returning a conditional type property instead of its own
type. See what we have for "ifBuilt".