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.
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.
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.
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.
(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.
(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
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)
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(-)
(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.
(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.
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(+)
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?
(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)
Created attachment 597920 [details] F0_problems.log.xz
(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.
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.
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(-)
(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.
(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.
