Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
> 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 Description: PGP signature
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
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?=
> 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
> ---
> 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
Description: This is a digitally signed message part
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
> 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?=
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
---
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
Description: PGP signature
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
On Thu, Dec 12, 2019 at 4:15 PM Ulrich Mueller wrote: > > > On Thu, 12 Dec 2019, Mike Gilbert wrote: > > > On Wed, Nov 27, 2019 at 11:14 PM Michał Górny 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
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
> On Thu, 12 Dec 2019, Mike Gilbert wrote: > On Wed, Nov 27, 2019 at 11:14 PM Michał Górny 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 Description: PGP signature
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
On Wed, Nov 27, 2019 at 11:14 PM Michał Górny 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.
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
> 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 Description: PGP signature
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
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 Description: This is a digitally signed message part
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
> 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 Description: PGP signature
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
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.
Re: [gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
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
> ---
> 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
Description: OpenPGP digital signature
[gentoo-portage-dev] [PATCH v2] eapply: Output verbosely only if patch fails to apply with -F0
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
---
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
