Re: Use RTC in Log4j and Log4Net

[Volkan Yazıcı]

On Wed, Sep 18, 2024 at 10:17 AM Piotr P. Karwasz 
wrote:

> However, we don't necessarily need to use `2.x` or `main` for those tests.
>

You cannot fix fuzz tests in another branch than `2.x` once OSS-Fuzz starts
pointing there – unless you're willing to waste +2 months for testing a
simple typo: waiting for a month on a OSS-Fuzz PR targeting from `2.x` to
`your-test-branch`, and another month for changing it back. I don't think
waiting +2 months for a one-liner is an acceptable development cycle.

Another example is INFRA issues. I forgot the count of arbitrary commits we
needed to push to `logging-site` to recover the last `logging.staged.a.o`
outage.

Re: Use RTC in Log4j and Log4Net

[Ralph Goers]

I sort of agree and sort of not. It would be nice to have automation that can 
enforce some rules. You can’t do that if there are none.

We could agree in principle to a small number of files and lines but set the 
enforcement to a larger number to allow for some exceptions. For example, if we 
agreed on 10 files max with no more than 5 lines each (50 lines total) we could 
set the enforcement at 20 files with a max of 200 lines (notice that is not 10 
lines per file).

Ralph

> On Sep 18, 2024, at 2:17 AM, Gary Gregory  wrote:
> 
> This proposal is two pages (on my phone)! I can't imagine counting lines
> and walking through these steps, it's crazy making IMO. How would you count
> lines anyway? From diff output?
> 
> I'd rather skip the bikeshedding and let devs create PRs when they think it
> makes sense. IOW, when they want a review. Otherwise, I trust devs to check
> their work.
> 
> Gary
> 
> On Wed, Sep 18, 2024, 3:46 AM Volkan Yazıcı  wrote:
> 
>> I agree in principle, but...
>> 
>> *Exemption criteria*
>> 
>> I agree with Ralph on that we should specify criteria on what shall be
>> exempt from this PR-driven workflow policy. We should of course materialize
>> these criteria collaboratively. Below is my attempt for a starting point:
>> 
>> Changes can be exempt from this policy if and only if they suffice *only
>> one* of the following conditions:
>> 
>>   1. Grammatical corrections to the documentation (incl. Javadoc and
>>   release notes)
>>   2. Code typo fixes¹ less than 10 LoC
>>   3. Infrastructure fixes¹ touching to `pom.xml` or CI scripts, and less
>>   than 10 LoC
>> 
>> ¹ A "fix" is assumed to not introduce a change in the expected behaviour of
>> the associated component. Changing the expected behaviour does not qualify
>> a fix.
>> 
>> *Scoped repositories*
>> 
>> I suggest extending the scope of this policy to cover the following
>> repositories too:
>> 
>>   1. `logging-parent`
>>   2. `logging-log4j-jakarta`
>>   3. `logging-jmx-gui`
>>   4. `logging-samples`
>>   5. `logging-site`
>>   6. `logging-log4net-site`
>> 
>> *Maintainer availability*
>> 
>> The PR-driven workflow can fly because we have full-time maintainers. But
>> that will not be the case anymore in 2-3 months due to funds drying out. I
>> suspect introducing such a policy with strict deadlines (e.g., 24 hours is
>> a pretty tight time budget for a project with no full-time maintainers) and
>> no exemptions might jeopardize the streamlining of development in the long
>> run. That said, as Christian noted, we can adapt/drop the policy later on
>> if needed.
>> 
>> I disagree with Matt's *"losing momentum from waiting for an unnecessary
>> code review"* statement. In the last three years or so – even before STF
>> funding and Piotr! – well-scoped PRs, where the author onboards reviewers
>> with sufficient navigation tips, have been processed very swiftly, AFAICT.
>> 
>> On Wed, Sep 18, 2024 at 12:16 AM Ralph Goers 
>> wrote:
>> 
>>> This isn’t a vote so I am not going to. If I had to vote I wouldn’t vote
>>> for a policy that requires RTC always. However, I would vote for a policy
>>> that requires RTC when specified criteria are met.
>>> 
>>> Ralph
>>> 
 On Sep 17, 2024, at 10:28 AM, Ralph Goers 
>>> wrote:
 
 First, the obvious. I haven’t committed much in a while. The last
>>> several I did I used PRs primarily because it makes it easier for people
>> to
>>> review the changes but I didn’t necessarily wait for a review.  For
>> really
>>> simple stuff I've never use a PR. However, with the switch from Jira to
>>> GitHub issues it might make more sense to use a PR since you have to have
>>> something to track the problem. But if there is already an issue then I
>>> would probably just use that. Again, depending on what is being done.
 
 I wouldn’t classify any of the work I’ve seen you doing recently as
>>> trivial or minor, so you only doing PRs makes sense.
 
 Ralph
 
> On Sep 17, 2024, at 7:52 AM, Piotr P. Karwasz <
>> piotr.karw...@gmail.com>
>>> wrote:
> 
> Hi Ralph,
> 
> On Tue, 17 Sept 2024 at 15:47, Ralph Goers <
>> ralph.go...@dslextreme.com>
>>> wrote:
>> 
>> Why? i.e. - what currently isn’t working?
> 
> I merely wish to formalize what is already happening and set up a
> branch protection rule to enforce it.
> 
> Note that I have never seen a PR in Log4Net being merged without a
>>> review.
> 
> On the other hand you can probably find a couple of my recent PRs that
> don't have a formal review. Sure, I must have certainly consulted with
> someone on Slack before I merged them, but there is no sign of it.
> Let's make it formal, so users also see it.
> 
> Piotr
 
>>> 
>>> 
>> 

Re: Use RTC in Log4j and Log4Net

[Matt Sicker]

The quick turnaround time for PR reviews is important when I only have time to 
work on the project one or two days a week.

> On Sep 18, 2024, at 08:38, Ralph Goers  wrote:
> 
> I sort of agree and sort of not. It would be nice to have automation that can 
> enforce some rules. You can’t do that if there are none.
> 
> We could agree in principle to a small number of files and lines but set the 
> enforcement to a larger number to allow for some exceptions. For example, if 
> we agreed on 10 files max with no more than 5 lines each (50 lines total) we 
> could set the enforcement at 20 files with a max of 200 lines (notice that is 
> not 10 lines per file).
> 
> Ralph
> 
>> On Sep 18, 2024, at 2:17 AM, Gary Gregory  wrote:
>> 
>> This proposal is two pages (on my phone)! I can't imagine counting lines
>> and walking through these steps, it's crazy making IMO. How would you count
>> lines anyway? From diff output?
>> 
>> I'd rather skip the bikeshedding and let devs create PRs when they think it
>> makes sense. IOW, when they want a review. Otherwise, I trust devs to check
>> their work.
>> 
>> Gary
>> 
>> On Wed, Sep 18, 2024, 3:46 AM Volkan Yazıcı  wrote:
>> 
>>> I agree in principle, but...
>>> 
>>> *Exemption criteria*
>>> 
>>> I agree with Ralph on that we should specify criteria on what shall be
>>> exempt from this PR-driven workflow policy. We should of course materialize
>>> these criteria collaboratively. Below is my attempt for a starting point:
>>> 
>>> Changes can be exempt from this policy if and only if they suffice *only
>>> one* of the following conditions:
>>> 
>>>  1. Grammatical corrections to the documentation (incl. Javadoc and
>>>  release notes)
>>>  2. Code typo fixes¹ less than 10 LoC
>>>  3. Infrastructure fixes¹ touching to `pom.xml` or CI scripts, and less
>>>  than 10 LoC
>>> 
>>> ¹ A "fix" is assumed to not introduce a change in the expected behaviour of
>>> the associated component. Changing the expected behaviour does not qualify
>>> a fix.
>>> 
>>> *Scoped repositories*
>>> 
>>> I suggest extending the scope of this policy to cover the following
>>> repositories too:
>>> 
>>>  1. `logging-parent`
>>>  2. `logging-log4j-jakarta`
>>>  3. `logging-jmx-gui`
>>>  4. `logging-samples`
>>>  5. `logging-site`
>>>  6. `logging-log4net-site`
>>> 
>>> *Maintainer availability*
>>> 
>>> The PR-driven workflow can fly because we have full-time maintainers. But
>>> that will not be the case anymore in 2-3 months due to funds drying out. I
>>> suspect introducing such a policy with strict deadlines (e.g., 24 hours is
>>> a pretty tight time budget for a project with no full-time maintainers) and
>>> no exemptions might jeopardize the streamlining of development in the long
>>> run. That said, as Christian noted, we can adapt/drop the policy later on
>>> if needed.
>>> 
>>> I disagree with Matt's *"losing momentum from waiting for an unnecessary
>>> code review"* statement. In the last three years or so – even before STF
>>> funding and Piotr! – well-scoped PRs, where the author onboards reviewers
>>> with sufficient navigation tips, have been processed very swiftly, AFAICT.
>>> 
>>> On Wed, Sep 18, 2024 at 12:16 AM Ralph Goers 
>>> wrote:
>>> 
 This isn’t a vote so I am not going to. If I had to vote I wouldn’t vote
 for a policy that requires RTC always. However, I would vote for a policy
 that requires RTC when specified criteria are met.
 
 Ralph
 
> On Sep 17, 2024, at 10:28 AM, Ralph Goers 
 wrote:
> 
> First, the obvious. I haven’t committed much in a while. The last
 several I did I used PRs primarily because it makes it easier for people
>>> to
 review the changes but I didn’t necessarily wait for a review.  For
>>> really
 simple stuff I've never use a PR. However, with the switch from Jira to
 GitHub issues it might make more sense to use a PR since you have to have
 something to track the problem. But if there is already an issue then I
 would probably just use that. Again, depending on what is being done.
> 
> I wouldn’t classify any of the work I’ve seen you doing recently as
 trivial or minor, so you only doing PRs makes sense.
> 
> Ralph
> 
>> On Sep 17, 2024, at 7:52 AM, Piotr P. Karwasz <
>>> piotr.karw...@gmail.com>
 wrote:
>> 
>> Hi Ralph,
>> 
>> On Tue, 17 Sept 2024 at 15:47, Ralph Goers <
>>> ralph.go...@dslextreme.com>
 wrote:
>>> 
>>> Why? i.e. - what currently isn’t working?
>> 
>> I merely wish to formalize what is already happening and set up a
>> branch protection rule to enforce it.
>> 
>> Note that I have never seen a PR in Log4Net being merged without a
 review.
>> 
>> On the other hand you can probably find a couple of my recent PRs that
>> don't have a formal review. Sure, I must have certainly consulted with
>> someone on Slack before I merged them, but there is no sign of it.
>> Let's make

Re: [log4j] Google OSS-Fuzz integration

[Matt Sicker]

Very neat!

> On Sep 17, 2024, at 14:21, Volkan Yazıcı  wrote:
> 
> OSS-Fuzz  is a Google service that
> continuously runs fuzz tests of critical F/OSS projects on a beefy cluster
> and reports its findings (bugs, vulnerabilities, etc.) privately to project
> maintainers. In #2949 ,
> I implemented fuzz tests for Log4j 2 and their integration with OSS-Fuzz. I
> have documented the details in `FUZZING.adoc`
> , e.g.,
> 
>   - How can I run fuzz tests locally?
>   - How can I view fuzzing failures detected by OSS-Fuzz?
>   - How can I reproduce fuzzing failures detected by OSS-Fuzz?
> 
> If you have any further questions, please let me know. If requested, I can
> also provide a walkthrough in the next PMC meeting.

Re: Use RTC in Log4j and Log4Net

[Gary Gregory]

This proposal is two pages (on my phone)! I can't imagine counting lines
and walking through these steps, it's crazy making IMO. How would you count
lines anyway? From diff output?

I'd rather skip the bikeshedding and let devs create PRs when they think it
makes sense. IOW, when they want a review. Otherwise, I trust devs to check
their work.

Gary

On Wed, Sep 18, 2024, 3:46 AM Volkan Yazıcı  wrote:

> I agree in principle, but...
>
> *Exemption criteria*
>
> I agree with Ralph on that we should specify criteria on what shall be
> exempt from this PR-driven workflow policy. We should of course materialize
> these criteria collaboratively. Below is my attempt for a starting point:
>
> Changes can be exempt from this policy if and only if they suffice *only
> one* of the following conditions:
>
>1. Grammatical corrections to the documentation (incl. Javadoc and
>release notes)
>2. Code typo fixes¹ less than 10 LoC
>3. Infrastructure fixes¹ touching to `pom.xml` or CI scripts, and less
>than 10 LoC
>
> ¹ A "fix" is assumed to not introduce a change in the expected behaviour of
> the associated component. Changing the expected behaviour does not qualify
> a fix.
>
> *Scoped repositories*
>
> I suggest extending the scope of this policy to cover the following
> repositories too:
>
>1. `logging-parent`
>2. `logging-log4j-jakarta`
>3. `logging-jmx-gui`
>4. `logging-samples`
>5. `logging-site`
>6. `logging-log4net-site`
>
> *Maintainer availability*
>
> The PR-driven workflow can fly because we have full-time maintainers. But
> that will not be the case anymore in 2-3 months due to funds drying out. I
> suspect introducing such a policy with strict deadlines (e.g., 24 hours is
> a pretty tight time budget for a project with no full-time maintainers) and
> no exemptions might jeopardize the streamlining of development in the long
> run. That said, as Christian noted, we can adapt/drop the policy later on
> if needed.
>
> I disagree with Matt's *"losing momentum from waiting for an unnecessary
> code review"* statement. In the last three years or so – even before STF
> funding and Piotr! – well-scoped PRs, where the author onboards reviewers
> with sufficient navigation tips, have been processed very swiftly, AFAICT.
>
> On Wed, Sep 18, 2024 at 12:16 AM Ralph Goers 
> wrote:
>
> > This isn’t a vote so I am not going to. If I had to vote I wouldn’t vote
> > for a policy that requires RTC always. However, I would vote for a policy
> > that requires RTC when specified criteria are met.
> >
> > Ralph
> >
> > > On Sep 17, 2024, at 10:28 AM, Ralph Goers 
> > wrote:
> > >
> > > First, the obvious. I haven’t committed much in a while. The last
> > several I did I used PRs primarily because it makes it easier for people
> to
> > review the changes but I didn’t necessarily wait for a review.  For
> really
> > simple stuff I've never use a PR. However, with the switch from Jira to
> > GitHub issues it might make more sense to use a PR since you have to have
> > something to track the problem. But if there is already an issue then I
> > would probably just use that. Again, depending on what is being done.
> > >
> > > I wouldn’t classify any of the work I’ve seen you doing recently as
> > trivial or minor, so you only doing PRs makes sense.
> > >
> > > Ralph
> > >
> > >> On Sep 17, 2024, at 7:52 AM, Piotr P. Karwasz <
> piotr.karw...@gmail.com>
> > wrote:
> > >>
> > >> Hi Ralph,
> > >>
> > >> On Tue, 17 Sept 2024 at 15:47, Ralph Goers <
> ralph.go...@dslextreme.com>
> > wrote:
> > >>>
> > >>> Why? i.e. - what currently isn’t working?
> > >>
> > >> I merely wish to formalize what is already happening and set up a
> > >> branch protection rule to enforce it.
> > >>
> > >> Note that I have never seen a PR in Log4Net being merged without a
> > review.
> > >>
> > >> On the other hand you can probably find a couple of my recent PRs that
> > >> don't have a formal review. Sure, I must have certainly consulted with
> > >> someone on Slack before I merged them, but there is no sign of it.
> > >> Let's make it formal, so users also see it.
> > >>
> > >> Piotr
> > >
> >
> >
>

Re: Use RTC in Log4j and Log4Net

[Piotr P. Karwasz]

Hi Volkan,

On Wed, 18 Sept 2024 at 09:46, Volkan Yazıcı  wrote:
> Changes can be exempt from this policy if and only if they suffice *only
> one* of the following conditions:
>
>1. Grammatical corrections to the documentation (incl. Javadoc and
>release notes)
>2. Code typo fixes¹ less than 10 LoC
>3. Infrastructure fixes¹ touching to `pom.xml` or CI scripts, and less
>than 10 LoC
>
> ¹ A "fix" is assumed to not introduce a change in the expected behaviour of
> the associated component. Changing the expected behaviour does not qualify
> a fix.

For Log4j I would also prefer 1 and 2 to go through a PR. It will be
easier for me to nag the author to port his changes to `main` this
way.
The port to `main` of a `2.x` PR probably does not require a PR.

Regarding 3, sure, debugging some GitHub actions problem is impossible
without pushing it to a branch.
However, we don't necessarily need to use `2.x` or `main` for those tests.

Piotr

Re: Use RTC in Log4j and Log4Net

[Volkan Yazıcı]

I agree in principle, but...

*Exemption criteria*

I agree with Ralph on that we should specify criteria on what shall be
exempt from this PR-driven workflow policy. We should of course materialize
these criteria collaboratively. Below is my attempt for a starting point:

Changes can be exempt from this policy if and only if they suffice *only
one* of the following conditions:

   1. Grammatical corrections to the documentation (incl. Javadoc and
   release notes)
   2. Code typo fixes¹ less than 10 LoC
   3. Infrastructure fixes¹ touching to `pom.xml` or CI scripts, and less
   than 10 LoC

¹ A "fix" is assumed to not introduce a change in the expected behaviour of
the associated component. Changing the expected behaviour does not qualify
a fix.

*Scoped repositories*

I suggest extending the scope of this policy to cover the following
repositories too:

   1. `logging-parent`
   2. `logging-log4j-jakarta`
   3. `logging-jmx-gui`
   4. `logging-samples`
   5. `logging-site`
   6. `logging-log4net-site`

*Maintainer availability*

The PR-driven workflow can fly because we have full-time maintainers. But
that will not be the case anymore in 2-3 months due to funds drying out. I
suspect introducing such a policy with strict deadlines (e.g., 24 hours is
a pretty tight time budget for a project with no full-time maintainers) and
no exemptions might jeopardize the streamlining of development in the long
run. That said, as Christian noted, we can adapt/drop the policy later on
if needed.

I disagree with Matt's *"losing momentum from waiting for an unnecessary
code review"* statement. In the last three years or so – even before STF
funding and Piotr! – well-scoped PRs, where the author onboards reviewers
with sufficient navigation tips, have been processed very swiftly, AFAICT.

On Wed, Sep 18, 2024 at 12:16 AM Ralph Goers 
wrote:

> This isn’t a vote so I am not going to. If I had to vote I wouldn’t vote
> for a policy that requires RTC always. However, I would vote for a policy
> that requires RTC when specified criteria are met.
>
> Ralph
>
> > On Sep 17, 2024, at 10:28 AM, Ralph Goers 
> wrote:
> >
> > First, the obvious. I haven’t committed much in a while. The last
> several I did I used PRs primarily because it makes it easier for people to
> review the changes but I didn’t necessarily wait for a review.  For really
> simple stuff I've never use a PR. However, with the switch from Jira to
> GitHub issues it might make more sense to use a PR since you have to have
> something to track the problem. But if there is already an issue then I
> would probably just use that. Again, depending on what is being done.
> >
> > I wouldn’t classify any of the work I’ve seen you doing recently as
> trivial or minor, so you only doing PRs makes sense.
> >
> > Ralph
> >
> >> On Sep 17, 2024, at 7:52 AM, Piotr P. Karwasz 
> wrote:
> >>
> >> Hi Ralph,
> >>
> >> On Tue, 17 Sept 2024 at 15:47, Ralph Goers 
> wrote:
> >>>
> >>> Why? i.e. - what currently isn’t working?
> >>
> >> I merely wish to formalize what is already happening and set up a
> >> branch protection rule to enforce it.
> >>
> >> Note that I have never seen a PR in Log4Net being merged without a
> review.
> >>
> >> On the other hand you can probably find a couple of my recent PRs that
> >> don't have a formal review. Sure, I must have certainly consulted with
> >> someone on Slack before I merged them, but there is no sign of it.
> >> Let's make it formal, so users also see it.
> >>
> >> Piotr
> >
>
>