Re: [webkit-dev] git-webkit pr now supports --no-ews / --ews flags

[Geoffrey Garen via webkit-dev]

This is great!

As it is said in "Ghost Dog: The Way of the Samurai", it is bad when one thing 
becomes two. Can we pick one name for this feature? Either —no-ews (git-webkit) 
and no-ews (GitHub) or —skip-ews (git-webkit) and skip-ews (GitHub)?

Thanks,
Geoff

> On Jan 26, 2023, at 11:36 AM, Aakash Jain via webkit-dev 
>  wrote:
> 
> Hi Everyone,
> 
> git-webkit pr now supports --no-ews / --ews flags (as of 259115@main). This 
> is similar to --no-ews flag we had in webkit-patch.  You can also manually 
> add skip-ews label on any PR when you don't need EWS to run anymore on it. 
> --no-ews flag will also automatically add the skip-ews label on the 
> corresponding GitHub PR.
> 
> Please feel free to use skip-ews GitHub label or --no-ews flag in git-webkit 
> pr, when you don't need to run EWS for any reason (e.g.: PR failed a critical 
> EWS indicating need for new iteration, and you don't need to run rest of the 
> EWSes on that commit).
> 
> If you notice any issues, please let me know or file bugs (and assign to me).
> 
> 
> Thanks
> Aakash
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Geoffrey Garen via webkit-dev]

Is the idea that a checker failure should block landing a patch, or just that 
it should start a conversation with your patch reviewer about why this case is 
an exception?

Maybe the ’should’ clause in the error message could clarify.

In general, I do think it’s helpful to flag any new raw pointer usage in a 
member variable. But I also agree that it might not be practical to get that 
number to true zero.

Thanks,
Geoff

> On Jan 21, 2023, at 12:12 PM, Michael Catanzaro via webkit-dev 
>  wrote:
> 
> On Fri, Jan 20 2023 at 06:15:38 PM -0800, Ryosuke Niwa via webkit-dev 
>  wrote:
>> Here´s a PR to make the style checker look for raw pointers & references in 
>> data members: https://github.com/WebKit/WebKit/pull/8907
>> I suggest we land this PR in 5 business days from now on unless someone 
>> objects.
> 
> I'm not sure how this would work as a style check rule since there's always 
> going to be exceptions. E.g. we probably don't want to convert GObject priv 
> pointers to use CheckedRef: that would be silly.
> 
> I think we can follow this rule in most of WebCore and maybe most of WebKit 
> and WTF as well. Probably not going to work for bmalloc. Not sure about JSC.
> 
> Michael
> 
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev



___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Compile times and class-scoped enums

[Geoffrey Garen via webkit-dev]

>> However, this change requires moving class-scoped enums out into the 
>> namespace scope.
> 
> Seems worthwhile. Doesn’t seem to me like it would have far reaching effects.

I agree.

> +using Type = DOMAudioSessionType;

Did you do this to make the patch smaller, or do you prefer this style?

Thanks,
Geoff___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Ben Nham is now a WebKit Reviewer

[Geoffrey Garen via webkit-dev]

Hi folks,

I’m happy to announce that Ben Nham is now a WebKit reviewer. 

Ben is an expert in many Apple technologies, and in Performance and WebPush in 
particular. Please feel free to reach out to Ben for reviews.

Thanks,
Geoff
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Proposal: Mandatory Commit and Merge Queue

[Geoffrey Garen via webkit-dev]

Thanks for gathering this data!

>> What are some notable cases of recent regressions that have landed because 
>> of non-use of commit queue and caused serious problems?
> 
> Some recent examples of regressions that would have been prevented by 
> mandatory commit/merge-queue as proposed:
> 
> https://commits.webkit.org/250940@main 
>  (PR did use Merge-Queue, but we’re 
> missing the feature that would have caught the problem)

What feature specifically?

> https://commits.webkit.org/250343@main 
>  (Commit did use Commit-Queue, but 
> skipped layout tests because of a previous iteration passed EWS)

Are you also proposing that merge-queue should re-run layout tests 
unconditionally? Do we know how long that takes on average? Do we have the 
hardware to support that?

> https://commits.webkit.org/250791@main 
>  (Should have failed EWS, but flakey 
> tests had different names, cross referencing trunk results would have made 
> that obvious)

How would merge-queue cross reference with trunk results or otherwise resolve 
this confusion?

> https://commits.webkit.org/248624@main 
>  (Landed manually, broke the build)

Seems like this would have been avoided by Merge-Queue alone. That’s nice.

For the other three cases, whether the proposal as-is would have resolved them 
or not seems muddy. Might need revision.

Thanks,
Geoff

>> Do you have any data on how frequent such regressions are, compared to the 
>> base rate of regressions that have landed despite use of commit queue?
> 
> Commit and Merge queue have basically prevented us from breaking the Mac 
> build, the only example of a broken Mac build I can find come from by-passing 
> Commit and Merge Queue. Layout test breakages are quite a bit more difficult 
> to reason about because breakages tend to not just be platform specific, but 
> configuration specific as well.
> 
>> 
>> Do you have any data on how frequently regressions are resolved by patches 
>> that land outside commit queue?
> 
> In the last week, we had 200 commits. 50 of those were made through the 
> Unsafe-Merge-Queue. Break down is here:
> 
> 15 were feature work that should have gone through Merge Queue
> 12 were test gardening
> 9 were build or test fixes
> 3 were 3rd party imports
> 3 were reverts
> 3 were tooling changes
> 2 were buildbot changes
> 2 were contributors.json changes
> 1 was a documentation change
> 
> My proposal would have sent the feature work, gardening work, imports, 
> tooling change and documentation change to Merge-Queue rather than 
> Unsafe-Merge-Queue. The build and test fixes would have needed modification 
> to their commit messages, but were the kind of changes I would want landed 
> via Unsafe-Merge-Queue.
> 
>> 
>>> and reduce demands of post-commit test gardening.
>> 
>> Is this goal distinct from preventing regressions from landing? If so, how?
> 
> I suppose I should have said:
> "The goal is to increase the stability of the build, speed up EWS and reduce 
> demands of post-commit test gardening by preventing regressions from 
> landing”, since build instability, slow EWS and post-commit test gardening 
> are all consequences of regressions landing.
> 
>> 
 What problem are you trying to solve, and with what level of urgency?
>>> 
>>> Urgency is not high. I started this with the expectation it would be a 
>>> somewhat long and contentious discussion. The motivating change is that the 
>>> GitHub transition makes this proposal possible, from a technical 
>>> perspective, in a way it is not while the project is still backed by 
>>> Subversion.
>> 
>> I don’t understand the premise here. There are lots of ways to enforce 
>> commit policy on a Subversion repository.
> 
> Kind of. The problem here is that we want to provide enough escape hatches so 
> that any committer can quickly repair a broken build, so we have to check not 
> just the committer, but the commit itself. This is more analogous to ensuring 
> that commit has “Reviewed by” in it’s commit message (something that 
> Subversion does not enforce in our repository, despite it being policy) than 
> it is to ensuring that the committer has certain privileges. We’ve always 
> implemented these kind of checks in buildbot, not the Subversion server, and 
> it’s much easier to implement complicated logic that takes into consideration 
> the content of the commit in buildbot than it is Subversion hooks.
> 
>> On the meta level, while we are still dealing with serious regressions in 
>> our workflow caused by git and GitHub, I recommend that we do not push 
>> forward with more unrelated sweeping changes to the project and its 
>> workflow. Just like in software development, where we need to fix 
>> regressions before we can move forward with major feature work, so too in 
>> tooling we need to do the same. Otherwise we just pile chaos 

Re: [webkit-dev] Deployment of new EWS Non-Unified builder

[Geoffrey Garen via webkit-dev]

> As such, I also think that the non-unified EWS being green should not be a 
> blocker to landing a patch. But I think having it there for information will 
> help the situation. At minimum, even if every engineer simply ignores the 
> non-unified EWS, it also makes it easier for someone trying to fix a trim 
> missing include build issue to scan through PRs to look for this EWS failure 
> in order to narrow down on which patches (and therefore possible includes) to 
> focus on.

Is this the proposal on the table — to have an EWS bot, but also not block 
patches on it? 

That’s surprising to me, and not how EWS bots usually work. If we just want an 
optional record of where a particular build configuration started failing, 
Isn’t that just… a not-EWS bot?

Thanks,
Geoff

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Proposal: Mandatory Commit and Merge Queue

[Geoffrey Garen via webkit-dev]

>> What is the goal of this proposal?
> 
> The goal is to increase the stability of the build

Is this goal distinct from preventing regressions from landing? If so, how?

> , speed up EWS by preventing regressions from landing

What are some notable cases of recent regressions that have landed because of 
non-use of commit queue and caused serious problems?

Do you have any data on how frequent such regressions are, compared to the base 
rate of regressions that have landed despite use of commit queue?

Do you have any data on how frequently regressions are resolved by patches that 
land outside commit queue?

> and reduce demands of post-commit test gardening.

Is this goal distinct from preventing regressions from landing? If so, how?

>> What problem are you trying to solve, and with what level of urgency?
> 
> Urgency is not high. I started this with the expectation it would be a 
> somewhat long and contentious discussion. The motivating change is that the 
> GitHub transition makes this proposal possible, from a technical perspective, 
> in a way it is not while the project is still backed by Subversion.

I don’t understand the premise here. There are lots of ways to enforce commit 
policy on a Subversion repository.

On the meta level, while we are still dealing with serious regressions in our 
workflow caused by git and GitHub, I recommend that we do not push forward with 
more unrelated sweeping changes to the project and its workflow. Just like in 
software development, where we need to fix regressions before we can move 
forward with major feature work, so too in tooling we need to do the same. 
Otherwise we just pile chaos on top of chaos, and there is no way to know if 
things are improving or getting worse, and no way to hold ourselves accountable 
for improvement. 

Thanks,
Geoff

> 
> Jonathan
> 
>> 
>> Thanks,
>> Geoff
>> 
>> 
>>> On Jun 2, 2022, at 2:35 PM, Jonathan Bedard via webkit-dev 
>>> mailto:webkit-dev@lists.webkit.org>> wrote:
>>> 
>>> As we move to GitHub, I would like to propose we strengthen our protections 
>>> on `main` by making MergeQueue and CommitQueue mandatory. This would mean 
>>> that with a few exceptions, all changes would need to be built and run 
>>> layout tests before they are landed. To spell out what the exceptions I had 
>>> in mind are:
>>> 
>>> - Revert commits, identified by a commit message that starts with 
>>> “Unreviewed, revering…” would be exempt
>>> - Changes which only modify files that do not effect building or testing 
>>> WebKit would be exempt. These files specifically are:
>>>  .github/
>>>  JSTests/
>>>  ManualTests/
>>>  metadata/
>>>  PerformanceTests/
>>>  Tools/
>>>  CISuport/
>>>  EWSTools/
>>>  WebKitBot
>>> Websites/
>>> - Emergency build and infrastructure fixes, identified by a commit message 
>>> that starts with “Emergency build fix” or “Emergency infrastructure fix” 
>>> would be exempt
>>> - A reviewer who is not the commit author can overwrite this protection by 
>>> adding `unsafe-merge-queue` instead of the commit author
>>> - Changes which passed an EWS layout test queue within the last 7 days 
>>> would skip the layout test check
>>> 
>>> These exceptions are designed to provide contributors for a way to by-pass 
>>> potentially slow checks if extraordinary situations, or in ones where CI 
>>> has already validated the change. I think we should keep the ability for 
>>> any committer to deploy an emergency fix, because our project has many 
>>> contributors in different timezones and with different holiday schedules.
>>> 
>>> We know that this policy change would potentially slow down development, so 
>>> I think these 3 improvements block making MergeQueue and CommitQueue 
>>> mandatory:
>>> 
>>> - run-webkit-tests consulting results.webkit.org 
>>>  to avoid retrying known flakey or failing tests
>>> - Another MergeQueue bot
>>> - Xcode workspace builds to speed up incremental builds
>>> 
>>> Jonathan Bedard
>>> WebKit Continuous Integration
>>> 
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org 
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Proposal: Mandatory Commit and Merge Queue

[Geoffrey Garen via webkit-dev]

> As we move to GitHub, I would like to propose we strengthen our protections 
> on `main` by making MergeQueue and CommitQueue mandatory. 


What is the goal of this proposal?

What problem are you trying to solve, and with what level of urgency?

Thanks,
Geoff


> On Jun 2, 2022, at 2:35 PM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> As we move to GitHub, I would like to propose we strengthen our protections 
> on `main` by making MergeQueue and CommitQueue mandatory. This would mean 
> that with a few exceptions, all changes would need to be built and run layout 
> tests before they are landed. To spell out what the exceptions I had in mind 
> are:
> 
> - Revert commits, identified by a commit message that starts with 
> “Unreviewed, revering…” would be exempt
> - Changes which only modify files that do not effect building or testing 
> WebKit would be exempt. These files specifically are:
>  .github/
>  JSTests/
>  ManualTests/
>  metadata/
>  PerformanceTests/
>  Tools/
>CISuport/
>EWSTools/
>WebKitBot
> Websites/
> - Emergency build and infrastructure fixes, identified by a commit message 
> that starts with “Emergency build fix” or “Emergency infrastructure fix” 
> would be exempt
> - A reviewer who is not the commit author can overwrite this protection by 
> adding `unsafe-merge-queue` instead of the commit author
> - Changes which passed an EWS layout test queue within the last 7 days would 
> skip the layout test check
> 
> These exceptions are designed to provide contributors for a way to by-pass 
> potentially slow checks if extraordinary situations, or in ones where CI has 
> already validated the change. I think we should keep the ability for any 
> committer to deploy an emergency fix, because our project has many 
> contributors in different timezones and with different holiday schedules.
> 
> We know that this policy change would potentially slow down development, so I 
> think these 3 improvements block making MergeQueue and CommitQueue mandatory:
> 
> - run-webkit-tests consulting results.webkit.org  
> to avoid retrying known flakey or failing tests
> - Another MergeQueue bot
> - Xcode workspace builds to speed up incremental builds
> 
> Jonathan Bedard
> WebKit Continuous Integration
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Proposal: Immediate Deprecation of ChangeLogs

[Geoffrey Garen via webkit-dev]

Do I undertand correctly that the proposal here is

(a) Immediately Deprecate ChangeLogs

(b) Immediately end support for posting patches from Subversion 
checkouts?

If so, do you know how many regular WebKit contributors still post patches from 
Subversion checkouts, and, if that number is not zero, what their schedule is 
for migrating to git, and whether they need anything from our tools engineers 
to make that migration smooth?

Seems… problematically forward-looking… to propose immediate migration without 
that data.

Thanks,
Geoff

> On May 10, 2022, at 1:32 PM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> A few weeks ago, I started a discussion about deprecating ChangeLogs. In that 
> time, we’ve had more folks using the pull-request workflow and more folks 
> using newer versions of `git` which break automatic ChangeLog rebasing. I 
> propose that on Monday, May 16th, we implement the following policy changes 
> for the WebKit project:
> 
> - Commits no longer require ChangeLogs, they instead require commit messages
> - Commit messages are in the format of `prepare-ChangeLog --no-write`
> 
> Pull-request workflows based on `git-webkit` already support this workflow 
> well, and `git-webkit setup` creates a `prepare-commit-msg` hook that will 
> appropriately format commit messages. In addition, `git format-patch` allows 
> us to create a patch which contains a commit message. This means that 
> contributors still using patch workflows from a git or git-svn checkout will 
> be able to upload compliant patches to bugzilla.
> 
> This will, however, break contributors using pure-Subversion checkouts. This 
> is something that’s going to be happening in the very near future as we 
> deprecate Subversion entirely, so I think this is an acceptable cost in 
> exchange for fully supporting native git workflows.
> 
> The last thing I’d like to note is that a full git-native commit message 
> policy now is something we can modify in the future if we find that reviewing 
> commit messages with “Quote reply” comments is not sufficient, but resolving 
> project disagreements on how or if to address deficiencies in GitHub commit 
> message review don’t seem to be headed towards a resolution quickly.
> 
> Jonathan
> WebKit Continuous Integration
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] ChangeLog Deprecation Plans

[Geoffrey Garen via webkit-dev]

>> Commit message is tied to a commit, so editing commit without breaking 
>> commit-message is hard (how to revert one change from one commit without 
>> rewriting commit message? It requires some git hack). Separate independent 
>> COMMIT_MESSAGE file can solve this problem and makes local development easy.
> 
> Developers who are used to git -- which nowadays is pretty much everyone 
> except WebKit devs -- are also used to rewriting commit messages.

I think it’s important for WebKit's git transition to take consideration of 
WebKit developers and WebKit workflows. I hope we can agree on this premise as 
we discuss various options for commit messages.

Thanks,
Geoff
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev