Bug 204725
| Summary: | Mk/bsd.port.mk: makepatch inserts context junk after r401709 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Jan Beich <jbeich> | ||||||
| Component: | Ports Framework | Assignee: | John Marino <marino> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Only Me | CC: | marino, portmgr, ports-bugs | ||||||
| Priority: | --- | Keywords: | needs-qa, patch, regression | ||||||
| Version: | Latest | Flags: | koobs:
maintainer-feedback?
(portmgr) |
||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
The sequence "clean patch makepatch" is wrong. There's a post-patch target that would affect the patches, so it should be "clean extract do-patch makepatch" There's a series of "locale: 2: bad variable name" messages that block most of the regenerations. Those would only appear on multipatch files. I'll take a look at the awk logic to see if there's an issue there. I'm not seeing your "false positives". It just failed to regenerate 8 of the 9 patches, so there's no point in even trying patch(1). It does say in the comments of makepatch that garbage in hunks can confuse it, but I don't think that's what is going on here. The issue here disappears after replacing [ -e ${COMMENTS}/${P} ] with false but then comments aren't preserved anymore. ports r402128 also lists other damage coming from |make makepatch|.
it's not counting hunk lines correctly. It keeps trying to eat hunk lines when there are no more. It's exclusive to multi-patch files. There were two issues. 1) The last minute add of "local" keyword broke makepatch. I don't know why, but some of the "local var=$(some cmd)" didn't work. I had to change them to: local var var=$(some cmd) 2) The hunk processing had a logic flaw. Please try the attached patch. I think it works. (It results in one less patch at the end because one patch changed a file that had been altered by another another patch. When this happens, the changes get combined and this is expected) Created attachment 163405 [details]
proposed patch to smart_makepatch.sh
Comment on attachment 163405 [details] proposed patch to smart_makepatch.sh Thanks. makepatch no longer inserts junk into patches. (In reply to John Marino from comment #4) > some of the "local var=$(some cmd)" didn't work Bug 166771? I don't see "locale: 2: bad variable name" even on FreeBSD 9.3R. (In reply to John Marino from comment #4) > ... the changes get combined and this is expected ... and any hunks applied before are *lost* because .orig file(s) now points to a patched version(s). If makepatch cannot avoid screwing up multi-file patches I cannot recommend it to anyone with clear conscience e.g., in bug 203353. One way to fix this issue is to employ a VCS to record changes then export them back. Dropping (any) timestamps in the process would be a feature, not a bug (less noise in commits). (In reply to Jan Beich from comment #6) I don't understand most of your response. The "local: 2: bad variable name" was the primary problem. It basically aborted the operation. However, I'm still on FreeBSD 9.2 on this test machine. "If makepatch cannot avoid screwing up multi-file patches ..." It clearly states both in the header and the commit message that if file X is modified by patch file A, B, and C, that it will combine all the changes and which patch it is appled to will vary. This is not "a screwup". If anything, the port maintainer could be blamed for allowing the situation, but sometimes it can't be helped if applying external patches. In such a case, a single patch may be temporarily moved before running "make makepatch" so the multipatch is generated the same way, and the single patch generated by hand. The assumption is the patches being regenerated do not apply without fuzz. ".. and any hunks applied before are *lost* because .orig file(s) now points to a patched version(s)" I'm not sure what you are talking about here. There are two ways to use makepatch. 1) regenerate (clean/extract/do-patch/makepatch) 2) update patches (clean/extract/do-patch/ <edit manually/new .orig>/makepatch There are no lost hunks. Unless you're still talking about a file that is patched multiple times (which again, should be avoided) "One way to fix this issue is to employ a VCS to record changes then export them back." I'm not interested in introducing a VCS requirement, nor do I see how that would even help. The "it can't restore multiple partial changes" issue is a rare use case that has a limited workaround and it's a publicized limitation. To tell somebody not to use a tool for a limitation that occurs around %0.1 of the ports ... how does that help anybody? However, I can't tell from your response. Does the attached patch cause "makepatch" to work as designed? to illustrate the workaround on a clean nspr: 1) cd /usr/ports/devel/nspr 2) mv files/patch-bug782111 to . 3) make clean extract do-patch makepatch 4) mv patch-bug782111 to files The result is identical to prior to the operation. Maintainers that have multiple patches for the same source file will need to do something like this if they want to maintain the split. That's known. A commit references this bug: Author: marino Date: Sun Nov 22 09:18:07 UTC 2015 New revision: 402220 URL: https://svnweb.freebsd.org/changeset/ports/402220 Log: Mk/Scripts/smart_makepatch.sh: Fix multi-patch file and locals bug There were two issues with the new smart_makepatch script. 1) use of "local" declaration All function variables were declared "local" during the review. This caused the script to break, at least on FreeBSD 9.2. Given that it's not being seen on 9.3R or later, it might be a bug in Bourne shell that has since been fixed. e.g. This resulted in stderr error on second iteration: local contains=$(grep "^+++ " ${existing_patch} | awk '{x++; print x}') however, this works fine: local contains contains=$(grep "^+++ " ${existing_patch} | awk '{x++; print x}') To be safe, all local variables are assigned with $(<shell cmd>) on separate lines now. 2) The comment extraction was flawed for files that contain multiple patches. It was not counting the hunk lines properly which caused some portion of a patch to be considered as a comment for the next patch. The hunk traversal algorithm has been fixed. Since 1) involved the introduction of local declarations that broke the script and since only Scripts/smart_makepatch.sh is touched, I will piggy-back on the original approval. The fix was tested with devel/nspr, the port listed in the PR, which uses multi-patch files. Approved by: portmgr Differential Revision: D4136 PR: 204725 Changes: head/Mk/Scripts/smart_makepatch.sh (In reply to John Marino from comment #7) Ah, I get what you mean by this now: ".. and any hunks applied before are *lost* because .orig file(s) now points to a patched version(s)" The first patch to File X creates the .orig file and the second patch to File X will overwrite it, thus the resulting patch only includes the last applied change. This will be difficult to fix because it's happening in the do-patch phase, well before makepatch comes in. It's actually another reason to avoid splitting a patch into multiple files. Maintainers have the same problem if they generate their patches manually (and the previous version of makepatch also did the the same thing) The do-patch step would have to be modified in a predictable way (e.g. on the first patch, cp <file>.orig to <file>.orig.1st if <file>.orig.1st does not exist. Then makepatch can mv any <file>.orig.1st back to <file>.orig before running. |
Created attachment 163394 [details] example junk Patch comment detection appears to be fragile and causes false positives. It doesn't break build only by accident: @@ offsets don't change, so patch(1) ignores out-of-bounds lines. For example, $ cd devel/nspr $ make clean patch makepatch $ svn diff