[Webkit-unassigned] [Bug 177225] Update syncing script to be able to build binary for commit set with owned commits.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=177225

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #321309|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170924/4a46bcbc/attachment-0001.html>