Bug 204725 - Mk/bsd.port.mk: makepatch inserts context junk after r401709
Summary: Mk/bsd.port.mk: makepatch inserts context junk after r401709
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: John Marino
URL:
Keywords: needs-qa, patch, regression
Depends on:
Blocks:
 
Reported: 2015-11-21 22:25 UTC by Jan Beich
Modified: 2015-11-22 09:28 UTC (History)
3 users (show)

See Also:
koobs: maintainer-feedback? (portmgr)


Attachments
example junk (4.68 KB, patch)
2015-11-21 22:25 UTC, Jan Beich
no flags Details | Diff
proposed patch to smart_makepatch.sh (2.53 KB, patch)
2015-11-22 01:57 UTC, John Marino
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer freebsd_triage 2015-11-21 22:25:18 UTC
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
Comment 1 John Marino freebsd_committer freebsd_triage 2015-11-21 22:57:05 UTC
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.
Comment 2 Jan Beich freebsd_committer freebsd_triage 2015-11-21 23:11:39 UTC
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|.
Comment 3 John Marino freebsd_committer freebsd_triage 2015-11-21 23:45:34 UTC
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.
Comment 4 John Marino freebsd_committer freebsd_triage 2015-11-22 01:56:48 UTC
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)
Comment 5 John Marino freebsd_committer freebsd_triage 2015-11-22 01:57:26 UTC
Created attachment 163405 [details]
proposed patch to smart_makepatch.sh
Comment 6 Jan Beich freebsd_committer freebsd_triage 2015-11-22 03:12:22 UTC
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).
Comment 7 John Marino freebsd_committer freebsd_triage 2015-11-22 08:27:36 UTC
(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?
Comment 8 John Marino freebsd_committer freebsd_triage 2015-11-22 08:39:20 UTC
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.
Comment 9 commit-hook freebsd_committer freebsd_triage 2015-11-22 09:18:29 UTC
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
Comment 10 John Marino freebsd_committer freebsd_triage 2015-11-22 09:28:43 UTC
(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.