Bug 177225 – Update syncing script to be able to build binary for commit set with owned commits.

• Status: NEW
• Component: New Bugs

Update syncing script to be able to build binary for commit set with owned commits.

Created attachment 321309 [details]
Patch

Comment on attachment 321309 [details]
Patch

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

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:678
> +
> +

I'll cleanup those unnecessary blank lines.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:961
> +

Probably should remove this blank line as welll.

[Ryosuke Niwa]

Comment on attachment 321309 [details]
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((repository) => { 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(commitSet)).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".

Created attachment 321808 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 321808 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/models/commit-set.js:72
> +    topLevelRepositoriesSortedByNamePreferringOnesWithURL() {

We can just call this topLevelRepositories() instead.
Nit: { should be on the next line.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:223
> +    topLevelRepositoriesSortedByNamePreferringOnesWithURL() {

Ditto.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:476
> +                case 'ownedRevisions':
> +                    return {type, ownerRepositories: value.map(resolveRepository)};

I don't think we need or should have a list of owner repositories.
For simplicity, just support a single owner instead.
e.g. we'd just specify iOS or macOS in our internal dashboard.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1007
> +                    'checkbox': {'ifRepositorySet': {'value': 'build-webkit', 'repositoriesToCheck': ['WebKit']}},

There's no need to have two nested dictionaries like this.
We should relax the restriction in _parseRepositoryGroupPropertyTemplate that option's dictionary can only contain a single key,
and specify value & repositoriesToCheck in the outer dictionary as in:
{'fRepositorySet': ['WebKit'], 'value': 'build-webkit'}

Comment on attachment 321808 [details]
Patch

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

>> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1007
>> +                    'checkbox': {'ifRepositorySet': {'value': 'build-webkit', 'repositoriesToCheck': ['WebKit']}},
> 
> There's no need to have two nested dictionaries like this.
> We should relax the restriction in _parseRepositoryGroupPropertyTemplate that option's dictionary can only contain a single key,
> and specify value & repositoriesToCheck in the outer dictionary as in:
> {'fRepositorySet': ['WebKit'], 'value': 'build-webkit'}

How do we determine which key of this dictionary should be used as first argument of makeOption?

Created attachment 322162 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 322162 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/models/commit-set.js:72
> +    topLevelRepositories() { return Repository.sortByNamePreferringOnesWithURL(this._repositories.filter((repository) => { return !this.ownerRevisionForRepository(repository); })); }

This is pretty long.
Could you split into multiple lines.
Also, you don't need { return ~ }. You can just do this._repositories.filter((repository) => !this.ownerRevisionForRepository(repository))

> Websites/perf.webkit.org/public/v3/models/commit-set.js:220
> +    topLevelRepositories() { return Repository.sortByNamePreferringOnesWithURL(this.repositories().filter((repository) => { return !this.ownerRevisionForRepository(repository); })); }

Ditto.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:285
> +                    const requiresBuild = value.repositoriesToCheck.map(commitSet.requiresBuildForRepository.bind(commitSet)).some((requiresBuild) => requiresBuild);

Just do repositoriesToCheck.some((commitSet) => commitSet.requiresBuildForRepository()) instead.