[PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Michał Górny-5
12d0c48ad disabled silent output for eapply, in order to obtain fuzz
factors in build logs.  However, this also causes eapply to report all
patched files which can make logs unreadable when there are no fuzz
factors to be reported.  Instead, use verbose output only when applying
the patch with -F0 fails.

To achieve that, attempt to apply each patch with -F0 --dry-run first.
If this succeeds, just silently apply the patch for real.  If it
doesn't, output an explicit eqawarn that the patch does not apply
cleanly and retry with the default fuzz factor and verbose output.
Non-silenced output applies both to successful application with fuzz
and to failure.

Signed-off-by: Michał Górny <[hidden email]>
---
 bin/phase-helpers.sh | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Changes in v2:
- added original path to output

diff --git a/bin/phase-helpers.sh b/bin/phase-helpers.sh
index 60f8d3243..b5691bd70 100644
--- a/bin/phase-helpers.sh
+++ b/bin/phase-helpers.sh
@@ -995,8 +995,20 @@ if ___eapi_has_eapply; then
  # -f to avoid interactivity
  # -g0 to guarantee no VCS interaction
  # --no-backup-if-mismatch not to pollute the sources
- ${patch_cmd} -p1 -f -g0 --no-backup-if-mismatch \
- "${patch_options[@]}" < "${f}"
+ local all_opts=(
+ -p1 -f -g0 --no-backup-if-mismatch
+ "${patch_options[@]}"
+ )
+ # try applying with -F0 first, output a verbose warning
+ # if fuzz factor is necessary
+ if ${patch_cmd} "${all_opts[@]}" --dry-run -s -F0 \
+ < "${f}" &>/dev/null; then
+ all_opts+=( -s -F0 )
+ else
+ eqawarn "    ${f}: patch failed to apply without a fuzz factor, please rebase"
+ fi
+
+ ${patch_cmd} "${all_opts[@]}" < "${f}"
  failed=${?}
  if ! eend "${failed}"; then
  __helpers_die "patch -p1 ${patch_options[*]} failed with ${f}"
--
2.24.0


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Zac Medico-2
On 11/27/19 11:17 AM, Michał Górny wrote:

> 12d0c48ad disabled silent output for eapply, in order to obtain fuzz
> factors in build logs.  However, this also causes eapply to report all
> patched files which can make logs unreadable when there are no fuzz
> factors to be reported.  Instead, use verbose output only when applying
> the patch with -F0 fails.
>
> To achieve that, attempt to apply each patch with -F0 --dry-run first.
> If this succeeds, just silently apply the patch for real.  If it
> doesn't, output an explicit eqawarn that the patch does not apply
> cleanly and retry with the default fuzz factor and verbose output.
> Non-silenced output applies both to successful application with fuzz
> and to failure.
>
> Signed-off-by: Michał Górny <[hidden email]>
> ---
>  bin/phase-helpers.sh | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> Changes in v2:
> - added original path to output
>
> diff --git a/bin/phase-helpers.sh b/bin/phase-helpers.sh
> index 60f8d3243..b5691bd70 100644
> --- a/bin/phase-helpers.sh
> +++ b/bin/phase-helpers.sh
> @@ -995,8 +995,20 @@ if ___eapi_has_eapply; then
>   # -f to avoid interactivity
>   # -g0 to guarantee no VCS interaction
>   # --no-backup-if-mismatch not to pollute the sources
> - ${patch_cmd} -p1 -f -g0 --no-backup-if-mismatch \
> - "${patch_options[@]}" < "${f}"
> + local all_opts=(
> + -p1 -f -g0 --no-backup-if-mismatch
> + "${patch_options[@]}"
> + )
> + # try applying with -F0 first, output a verbose warning
> + # if fuzz factor is necessary
> + if ${patch_cmd} "${all_opts[@]}" --dry-run -s -F0 \
> + < "${f}" &>/dev/null; then
> + all_opts+=( -s -F0 )
> + else
> + eqawarn "    ${f}: patch failed to apply without a fuzz factor, please rebase"
> + fi
> +
> + ${patch_cmd} "${all_opts[@]}" < "${f}"
>   failed=${?}
>   if ! eend "${failed}"; then
>   __helpers_die "patch -p1 ${patch_options[*]} failed with ${f}"
>
Looks good. Please merge.
--
Thanks,
Zac


signature.asc (1000 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Michael Orlitzky
In reply to this post by Michał Górny-5
On 11/27/19 2:17 PM, Michał Górny wrote:
>
> To achieve that, attempt to apply each patch with -F0 --dry-run first.
> If this succeeds, just silently apply the patch for real.  If it
> doesn't, output an explicit eqawarn that the patch does not apply
> cleanly and retry with the default fuzz factor and verbose output.

This now disagrees with the PMS algorithm, doesn't it? Not that the
change isn't sensible otherwise.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Ulrich Mueller-2
>>>>> On Wed, 27 Nov 2019, Michael Orlitzky wrote:

> This now disagrees with the PMS algorithm, doesn't it?

The difference is that there is now a QA warning for something that is
perfectly within the spec. Maybe the extra verboseness would be enough,
but the eqawarn line should be omitted? It doesn't provide any info that
isn't already present in the output of patch itself.

Ulrich

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Michał Górny-5
On Wed, 2019-11-27 at 23:49 +0100, Ulrich Mueller wrote:
> > > > > > On Wed, 27 Nov 2019, Michael Orlitzky wrote:
> > This now disagrees with the PMS algorithm, doesn't it?
>
> The difference is that there is now a QA warning for something that is
> perfectly within the spec. Maybe the extra verboseness would be enough,
> but the eqawarn line should be omitted? It doesn't provide any info that
> isn't already present in the output of patch itself.
>

It helps people understand why some patches throw a wall of text while
others don't.

--
Best regards,
Michał Górny


signature.asc (631 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Ulrich Mueller-2
>>>>> On Thu, 28 Nov 2019, Michał Górny wrote:

>> The difference is that there is now a QA warning for something that
>> is perfectly within the spec. Maybe the extra verboseness would be
>> enough, but the eqawarn line should be omitted? It doesn't provide
>> any info that isn't already present in the output of patch itself.

> It helps people understand why some patches throw a wall of text while
> others don't.

WFM, but then omit the "please rebase" at least.

(Also, with the filename added, the line tends to be very long.
Maybe omit that leading whitespace to at least partially compensate?)

Ulrich

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Mike Gilbert-2
In reply to this post by Michał Górny-5
On Wed, Nov 27, 2019 at 11:14 PM Michał Górny <[hidden email]> wrote:

>
> On Wed, 2019-11-27 at 23:49 +0100, Ulrich Mueller wrote:
> > > > > > > On Wed, 27 Nov 2019, Michael Orlitzky wrote:
> > > This now disagrees with the PMS algorithm, doesn't it?
> >
> > The difference is that there is now a QA warning for something that is
> > perfectly within the spec. Maybe the extra verboseness would be enough,
> > but the eqawarn line should be omitted? It doesn't provide any info that
> > isn't already present in the output of patch itself.
> >
>
> It helps people understand why some patches throw a wall of text while
> others don't.

It also triggers pointless bug reports. Please remove this.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Ulrich Mueller-2
>>>>> On Thu, 12 Dec 2019, Mike Gilbert wrote:

> On Wed, Nov 27, 2019 at 11:14 PM Michał Górny <[hidden email]> wrote:
>> On Wed, 2019-11-27 at 23:49 +0100, Ulrich Mueller wrote:

>> > The difference is that there is now a QA warning for something that
>> > is perfectly within the spec. Maybe the extra verboseness would be
>> > enough, but the eqawarn line should be omitted? It doesn't provide
>> > any info that isn't already present in the output of patch itself.
>>
>> It helps people understand why some patches throw a wall of text
>> while others don't.

> It also triggers pointless bug reports. Please remove this.

I don't like that eqawarn either (see above).

OTOH, users shouldn't normally have "qa" in PORTAGE_ELOG_CLASSES,
so they won't see the warning?

Ulrich

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Mike Gilbert-2
On Thu, Dec 12, 2019 at 4:15 PM Ulrich Mueller <[hidden email]> wrote:

>
> >>>>> On Thu, 12 Dec 2019, Mike Gilbert wrote:
>
> > On Wed, Nov 27, 2019 at 11:14 PM Michał Górny <[hidden email]> wrote:
> >> On Wed, 2019-11-27 at 23:49 +0100, Ulrich Mueller wrote:
>
> >> > The difference is that there is now a QA warning for something that
> >> > is perfectly within the spec. Maybe the extra verboseness would be
> >> > enough, but the eqawarn line should be omitted? It doesn't provide
> >> > any info that isn't already present in the output of patch itself.
> >>
> >> It helps people understand why some patches throw a wall of text
> >> while others don't.
>
> > It also triggers pointless bug reports. Please remove this.
>
> I don't like that eqawarn either (see above).
>
> OTOH, users shouldn't normally have "qa" in PORTAGE_ELOG_CLASSES,
> so they won't see the warning?

Here's a bug report filed by a user, which is what prompted me to
reply on this thread in the first place.

https://bugs.gentoo.org/702608

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Ulrich Mueller-2
>>>>> On Fri, 13 Dec 2019, Mike Gilbert wrote:

>> > It also triggers pointless bug reports. Please remove this.
>>
>> I don't like that eqawarn either (see above).
>>
>> OTOH, users shouldn't normally have "qa" in PORTAGE_ELOG_CLASSES,
>> so they won't see the warning?

> Here's a bug report filed by a user, which is what prompted me to
> reply on this thread in the first place.

> https://bugs.gentoo.org/702608

Well then, trivial patch included below.


From 81000b32d330a1cc41a4541f7e4264918eb7e6c5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <[hidden email]>
Date: Fri, 13 Dec 2019 23:41:23 +0100
Subject: [PATCH] eapply: Drop QA warning for fuzz factor.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This didn't add any information beyond what is already present in the
output of patch. Developers will know how to interpret its output, and
users won't see the warning anyway with the standard configuration.

Signed-off-by: Ulrich Müller <[hidden email]>
---
 bin/phase-helpers.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/bin/phase-helpers.sh b/bin/phase-helpers.sh
index b5691bd70..020862ba0 100644
--- a/bin/phase-helpers.sh
+++ b/bin/phase-helpers.sh
@@ -1004,8 +1004,6 @@ if ___eapi_has_eapply; then
  if ${patch_cmd} "${all_opts[@]}" --dry-run -s -F0 \
  < "${f}" &>/dev/null; then
  all_opts+=( -s -F0 )
- else
- eqawarn "    ${f}: patch failed to apply without a fuzz factor, please rebase"
  fi
 
  ${patch_cmd} "${all_opts[@]}" < "${f}"
--
2.24.0

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Michał Górny-5
On Fri, 2019-12-13 at 23:49 +0100, Ulrich Mueller wrote:

> > > > > > On Fri, 13 Dec 2019, Mike Gilbert wrote:
> > > > It also triggers pointless bug reports. Please remove this.
> > >
> > > I don't like that eqawarn either (see above).
> > >
> > > OTOH, users shouldn't normally have "qa" in PORTAGE_ELOG_CLASSES,
> > > so they won't see the warning?
> > Here's a bug report filed by a user, which is what prompted me to
> > reply on this thread in the first place.
> > https://bugs.gentoo.org/702608
>
> Well then, trivial patch included below.
>
>
> From 81000b32d330a1cc41a4541f7e4264918eb7e6c5 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <[hidden email]>
> Date: Fri, 13 Dec 2019 23:41:23 +0100
> Subject: [PATCH] eapply: Drop QA warning for fuzz factor.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> This didn't add any information beyond what is already present in the
> output of patch. Developers will know how to interpret its output, and
> users won't see the warning anyway with the standard configuration.
>
> Signed-off-by: Ulrich Müller <[hidden email]>
> ---
>  bin/phase-helpers.sh | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/bin/phase-helpers.sh b/bin/phase-helpers.sh
> index b5691bd70..020862ba0 100644
> --- a/bin/phase-helpers.sh
> +++ b/bin/phase-helpers.sh
> @@ -1004,8 +1004,6 @@ if ___eapi_has_eapply; then
>   if ${patch_cmd} "${all_opts[@]}" --dry-run -s -F0 \
>   < "${f}" &>/dev/null; then
>   all_opts+=( -s -F0 )
> - else
> - eqawarn "    ${f}: patch failed to apply without a fuzz factor, please rebase"
>   fi
>  
>   ${patch_cmd} "${all_opts[@]}" < "${f}"
Actually, I added that because of your comment that people should be
rebasing patches rather than removing context.

--
Best regards,
Michał Górny


signature.asc (631 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0

Ulrich Mueller-2
>>>>> On Sat, 14 Dec 2019, Michał Górny wrote:

> Actually, I added that because of your comment that people should be
> rebasing patches rather than removing context.

Isn't rebasing easier than removing context, anyway? I'd trust the
maintainer to do the right thing there.

The main argument is that the warning is apparently seen by some users,
who will then file unwanted bug reports. (Also, as I said before, we
don't even have a policy that patches must apply without fuzz.)

Ulrich

signature.asc (497 bytes) Download Attachment