Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 674562 - [Future EAPI] Enable patch -F0 by default
Summary: [Future EAPI] Enable patch -F0 by default
Status: RESOLVED WONTFIX
Alias: None
Product: Gentoo Hosted Projects
Classification: Unclassified
Component: PMS/EAPI (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: PMS/EAPI
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: future-eapi
  Show dependency tree
 
Reported: 2019-01-04 22:59 UTC by Sergei Trofimovich (RETIRED)
Modified: 2020-03-04 17:40 UTC (History)
6 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
F0_problems.log.xz (F0_problems.log.xz,218.65 KB, application/x-xz)
2019-11-29 20:28 UTC, Sergei Trofimovich (RETIRED)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich (RETIRED) gentoo-dev 2019-01-04 22:59:49 UTC
In https://bugs.gentoo.org/674538 patch with small context=2 was applied on top of already applied source. This caused double-patching.

http://savannah.gnu.org/bugs/index.php?55393 suggested passing -F0 to avoid such situations.

I suggest adding -F0 to eapply's arguments for EAPI=8 and later.
Comment 1 Mike Gilbert gentoo-dev 2019-01-04 23:35:05 UTC
I think it is fairly common for ebuilds to apply patches that require a non-zero fuzz factor. Passing -F0 by default sounds like a pain in the ass.
Comment 2 Sergei Trofimovich (RETIRED) gentoo-dev 2019-01-05 12:46:23 UTC
The concern sounds reasonable.

I'll run comparison test for
    'eapply'/'eapply -F0'
test to see how many new ebuilds will fail. And will check how many of them are invalid patches applied.
Comment 3 Ulrich Müller gentoo-dev 2019-01-05 13:51:02 UTC
A default of -F0 would break many patches from upstream if they're not on top of the exact version of the package.

If you want to apply patches with a small context (like 2 in your example), you can always pass an explicit -F option to eapply. Otherwise, I believe that upstream defaults for diff (3 context lines) and patch (fuzz factor of 2) are just fine, and are what people will expect.
Comment 4 Sergei Trofimovich (RETIRED) gentoo-dev 2019-01-05 14:27:55 UTC
(In reply to Ulrich Müller from comment #3)
> A default of -F0 would break many patches from upstream if they're not on
> top of the exact version of the package.

The fuzz numbers are arbitrary. It allows a subset o patches to apply even if they are not precise. The original report shows when it's bad. Sure it can also happen to be good sometimes. What is a safe enough default is a hard question to answer.

I'll grab some numbers of failed patches for ::gentoo in a few days.

So far I found at least
    app-admin/gkrellm/gkrellm-2.3.10-r1.ebuild
where gkrellm-2.3.5-cifs.patch silently applies something that is already upstreamed.

> If you want to apply patches with a small context (like 2 in your example),
> you can always pass an explicit -F option to eapply. Otherwise, I believe
> that upstream defaults for diff (3 context lines) and patch (fuzz factor of
> 2) are just fine, and are what people will expect.

"what people will expect" is fine for interactive use. I disagree it's fine for automatic use like eapply.

To pass an option to eapply I need to know first that patch is applied incorrectly. I'l have to use -F0 proactively in all my packages.
Comment 5 Lars Wendler (Polynomial-C) gentoo-dev 2019-01-11 09:30:10 UTC
(In reply to Sergei Trofimovich from comment #4)
> 
> So far I found at least
>     app-admin/gkrellm/gkrellm-2.3.10-r1.ebuild
> where gkrellm-2.3.5-cifs.patch silently applies something that is already
> upstreamed.
> 

Thanks. Fixed:
https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=44ced7fbb5916662cdbd7d17aa3206343fd3fe06
Comment 6 Ulrich Müller gentoo-dev 2019-01-11 10:29:59 UTC
Maybe eapply shouldn't pass -s to patch? It is contrary to what we do elsewhere (e.g. --disable-silent-rules), and traditionally epatch didn't have it either. That way, any fuzz would at least be reported.

(Fortunately, -s isn't mandated by the spec: https://archives.gentoo.org/gentoo-pms/message/61e14dfbb96472454c4f60a6b1e90264)
Comment 7 Larry the Git Cow gentoo-dev 2019-11-25 16:59:51 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/proj/portage.git/commit/?id=12d0c48addaf4506dfd7b33f85f2441618a46dd2

commit 12d0c48addaf4506dfd7b33f85f2441618a46dd2
Author:     Ulrich Müller <ulm@gentoo.org>
AuthorDate: 2019-11-25 13:03:13 +0000
Commit:     Ulrich Müller <ulm@gentoo.org>
CommitDate: 2019-11-25 13:03:13 +0000

    eapply: Drop -s option for patch.
    
    We generally try to have verbose build logs, e.g., by calling
    configure with --disable-silent-rules. Silencing patch contradicts
    this, and will suppress reporting of fuzz factors.
    
    Note that the eapply specification in PMS calls patch without -s:
    https://projects.gentoo.org/pms/7/pms.html#x1-127001r1
    Traditionally, the -s option wasn't used by epatch either.
    
    Bug: https://bugs.gentoo.org/674562
    Signed-off-by: Ulrich Müller <ulm@gentoo.org>

 bin/phase-helpers.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Comment 8 Ulrich Müller gentoo-dev 2019-11-25 17:10:18 UTC
(In reply to Sergei Trofimovich from comment #2)
> The concern sounds reasonable.
> 
> I'll run comparison test for
>     'eapply'/'eapply -F0'
> test to see how many new ebuilds will fail. And will check how many of them
> are invalid patches applied.

Any results?

I must say that I am very sceptical about applying the proposed change, unless we know what would be its impact.
Comment 9 Sergei Trofimovich (RETIRED) gentoo-dev 2019-11-26 22:09:41 UTC
(In reply to Ulrich Müller from comment #8)
> (In reply to Sergei Trofimovich from comment #2)
> > The concern sounds reasonable.
> > 
> > I'll run comparison test for
> >     'eapply'/'eapply -F0'
> > test to see how many new ebuilds will fail. And will check how many of them
> > are invalid patches applied.
> 
> Any results?

I'm running another attempt to get tree-wide stats right now. Should take a few days.

Apparently running src_prepare() in a reliable way without hangups is a hard problem.
Comment 10 Larry the Git Cow gentoo-dev 2019-11-27 03:45:07 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=3e36007163e56015301e2146c671fb14b3b0af57

commit 3e36007163e56015301e2146c671fb14b3b0af57
Author:     Zac Medico <zmedico@gentoo.org>
AuthorDate: 2019-11-27 03:39:22 +0000
Commit:     Zac Medico <zmedico@gentoo.org>
CommitDate: 2019-11-27 03:39:57 +0000

    sys-apps/portage: Bump to version 2.3.80
    
     #667432 Rename DCO_SIGNED_OFF_BY config variable to SIGNED_OFF_BY.
     #674562 eapply: Drop -s option for patch.
     #689226 emerge --buildpkgonly: respect buildtime hard blockers
     #699986 emerge: add --quickpkg-direct option
    
    Bug: https://bugs.gentoo.org/701268
    Bug: https://bugs.gentoo.org/667432
    Bug: https://bugs.gentoo.org/674562
    Bug: https://bugs.gentoo.org/689226
    Bug: https://bugs.gentoo.org/699986
    Package-Manager: Portage-2.3.80, Repoman-2.3.18
    Signed-off-by: Zac Medico <zmedico@gentoo.org>

 sys-apps/portage/Manifest              |   1 +
 sys-apps/portage/portage-2.3.80.ebuild | 261 +++++++++++++++++++++++++++++++++
 2 files changed, 262 insertions(+)
Comment 11 Lars Wendler (Polynomial-C) gentoo-dev 2019-11-27 12:25:56 UTC
Sorry guys but I don't agree with dropping -s because in other areas we already are more verbose.
Before this change I had a simple nice overview wich patches got applied and in which order.
Now I get a huge output from each applied patch to see what files have been touched. This makes tracking of applied patches quite cumbersome and annoying.

Can we please have some kind of switch for eapply to make it non-verbose again without the need to create a custom src_prepare() function for each ebuild?
Comment 12 Sergei Trofimovich (RETIRED) gentoo-dev 2019-11-29 20:26:59 UTC
(In reply to Ulrich Müller from comment #8)
> (In reply to Sergei Trofimovich from comment #2)
> > The concern sounds reasonable.
> >
> > I'll run comparison test for
> >     'eapply'/'eapply -F0'
> > test to see how many new ebuilds will fail. And will check how many of them
> > are invalid patches applied.
>
> Any results?
>
> I must say that I am very sceptical about applying the proposed change,
> unless we know what would be its impact.

Finally fetched the result.

Summary is:

- 32785 OK (92%): unpacked fine
-  1934 SKIP (5%): unknown (src_prepare() requirements are not met)
-  1014 FAILED (3%): will fail with -F0 when F2 succeeded

Note: '32785 OK' are all ebuilds. It's a bit hard to say precisely which ebuilds   use any patches without evaluating them, but we can approximate:
    $ git grep -l -E 'eapply|epatch|PATCHES' -- '*/*/*.ebuild' | wc -l
    9427

Against that number the breakdown is:
- 1934 SKIP (10%): unknown (src_prepare() requirements are not met)
Comment 13 Sergei Trofimovich (RETIRED) gentoo-dev 2019-11-29 20:28:05 UTC
Created attachment 597920 [details]
F0_problems.log.xz
Comment 14 Ulrich Müller gentoo-dev 2019-11-29 20:59:07 UTC
(In reply to Sergei Trofimovich from comment #12)
> Finally fetched the result.

Thanks!

> Summary is:
> 
> - 32785 OK (92%): unpacked fine
> -  1934 SKIP (5%): unknown (src_prepare() requirements are not met)
> -  1014 FAILED (3%): will fail with -F0 when F2 succeeded
> 
> Note: '32785 OK' are all ebuilds. It's a bit hard to say precisely which
> ebuilds   use any patches without evaluating them, but we can approximate:
>     $ git grep -l -E 'eapply|epatch|PATCHES' -- '*/*/*.ebuild' | wc -l
>     9427
> 
> Against that number the breakdown is:
> - 1934 SKIP (10%): unknown (src_prepare() requirements are not met)

So IIUC, the base number is about 10000 ebuilds that apply patches, and about 1000 of them will fail with -F0 (but succeed with the -F2 default)?

Since that is a significant percentage, I'd suggest that we stay with the default (-F2). The recently added -F0 --dry-run code will at least output a verbose warning for patches that will only apply with a fuzz factor:
https://gitweb.gentoo.org/proj/portage.git/commit/?id=4a315c41b43867320b27e882bb1427ac553e683c

IMHO that should be sufficient to make maintainers aware of potential problems.
Comment 15 Sergei Trofimovich (RETIRED) gentoo-dev 2020-03-04 13:31:05 UTC
Double patch application happened again. This time in net-misc/ntp/ntp-4.2.8_p14.ebuild, patch "${FILESDIR}"/${PN}-4.2.8-gc-tests.patch #564018. Patch was upstreamed as-is, but patch did not find enough difference and applied it twice.
Comment 16 Larry the Git Cow gentoo-dev 2020-03-04 13:40:30 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=e744aedf9ec1e2f374acebe42b6d7482e1324618

commit e744aedf9ec1e2f374acebe42b6d7482e1324618
Author:     Sergei Trofimovich <slyfox@gentoo.org>
AuthorDate: 2020-03-04 13:40:03 +0000
Commit:     Sergei Trofimovich <slyfox@gentoo.org>
CommitDate: 2020-03-04 13:40:24 +0000

    net-misc/ntp: drop upstreamed 4.2.8-gc-tests.patch, bug #564018
    
    Noticed when tested upstream fix at https://bugs.ntp.org/show_bug.cgi?id=3601
    
    Bug: https://bugs.gentoo.org/674562
    Package-Manager: Portage-2.3.89, Repoman-2.3.20
    Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>

 net-misc/ntp/ntp-4.2.8_p14.ebuild | 1 -
 1 file changed, 1 deletion(-)
Comment 17 Ulrich Müller gentoo-dev 2020-03-04 13:59:27 UTC
(In reply to Sergei Trofimovich from comment #15)
> Double patch application happened again. This time in
> net-misc/ntp/ntp-4.2.8_p14.ebuild, patch
> "${FILESDIR}"/${PN}-4.2.8-gc-tests.patch #564018. Patch was upstreamed
> as-is, but patch did not find enough difference and applied it twice.

Portage (2.3.89-r1) reports the fuzz factor here:

 * Applying ntp-4.2.8-gc-tests.patch ...
patching file sntp/m4/ntp_problemtests.m4
Hunk #1 succeeded at 37 with fuzz 2 (offset 4 lines).
Hunk #2 succeeded at 55 with fuzz 2 (offset 8 lines).
Hunk #3 succeeded at 73 with fuzz 2 (offset 12 lines).                   [ ok ]


(In reply to Ulrich Müller from comment #14)
> IMHO that should be sufficient to make maintainers aware of potential
> problems.

^^ This.
Comment 18 Sergei Trofimovich (RETIRED) gentoo-dev 2020-03-04 17:40:48 UTC
(In reply to Ulrich Müller from comment #17)
> (In reply to Sergei Trofimovich from comment #15)
> > Double patch application happened again. This time in
> > net-misc/ntp/ntp-4.2.8_p14.ebuild, patch
> > "${FILESDIR}"/${PN}-4.2.8-gc-tests.patch #564018. Patch was upstreamed
> > as-is, but patch did not find enough difference and applied it twice.
> 
> Portage (2.3.89-r1) reports the fuzz factor here:
> 
>  * Applying ntp-4.2.8-gc-tests.patch ...
> patching file sntp/m4/ntp_problemtests.m4
> Hunk #1 succeeded at 37 with fuzz 2 (offset 4 lines).
> Hunk #2 succeeded at 55 with fuzz 2 (offset 8 lines).
> Hunk #3 succeeded at 73 with fuzz 2 (offset 12 lines).                   [
> ok ]
> 
> 
> (In reply to Ulrich Müller from comment #14)
> > IMHO that should be sufficient to make maintainers aware of potential
> > problems.
> 
> ^^ This.

OK. I interpret that "IMHO" as a rejection by PMS team.