Page MenuHomeFreeBSD

Greatly enhance "make makepatch" (address 2 major deficiencies)
ClosedPublic

Authored by marino on Nov 12 2015, 2:54 PM.

Details

Summary

Based on a continual stream of complaints about portlint which
partially stemmed from weakness in makepatch, I decided to address those
complaints once and for all.

This new version of makepatch:

  1. conserves comments in existing patches
  2. Works on patch files that contain multiple patches. It will reassemble multipatch files after generating them
  3. Will never, ever, change the name of an existing patch. No matter what the patch name is, even if it's never followed any convention, the patch will be regenerated in place with the same name
  4. The old patches are all archived (not destroyed)
  5. Any patch not regenerated is deleted.

Limitations:
A) If a source file is modified by multiple patches, the regenerated set

will combine the partial changes to a single change.

B) It may get confused on multi-patch files if the patch itself is corrupt.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1115
Build 1119: arc lint + arc unit

Event Timeline

marino retitled this revision from to Greatly enhance "make makepatch" (address 2 major deficiencies).
marino updated this object.
marino added a reviewer: danfe.

I abhor the idea (and current behaviour) of having old patches in some format, with new patches with a completely different filename format as the default.

That preservation behaviour should be a different non-default target (or something): makepatch-preserve. The reason we're in this mess in the first case is a lack of consistency. That the tooling will changed to accomodate/perpetuate the inconsistency is beyond me.

I want my ports consistent, and up-to-modern-date standards, and I want my tooling to tell me correctly (without false positives) what needs to be done, so I can do it, period.

If this means relaxing or removing portlint checks, so be it.

to remind folks:

the current "make makepatch" preserves names when the separator is "+", "-", or "_". If it doesn't match, the "standard" name is used.

The script above can be modified to retain the current behavior, which was the point of a long discussion (and represents a compromise)

marino edited edge metadata.

Restore handling of non-standard patch names

This alteration makes the script rename the patch to the standard patch
name if it doesn't match the legacy separators (_,-,+). The exception
to this is made on multi-patch files. For these, the patch name is
maintained in all cases.

While here, improve the feedback to user so he/she knows if the
regenerated patch has a legacy name or a new one.

marino edited edge metadata.

Handle case where patches were generated with "./" prefix

The "./" prefix was breaking the legacy name case and even messing
up the standard file name. This change should handle it now. I also
added a missing (but harmlessly missing) break in the legacy name checker.

marino edited edge metadata.

This time with the actual changes (see prior commit for log)

I think this is complete, so I'm not intending on updating D4136 anymore unless the review dictates it. It's ready for review now.

In the end, this is just additional functionality. For basic patches the functionality is identical, so this should not be a contraversial review IMO.

bdrewery added inline comments.
Mk/Scripts/smart_makepatch.sh
48

Can you mark variables with local please?

marino edited edge metadata.

Declare all function variables as locale

The only global variable is "old_patch_list"

mat added inline comments.
Mk/Scripts/smart_makepatch.sh
34

While since rP400846 no port should ever write any files in WRKDIR and everything should be done in WRKSRC (and other subdirectories in WRKDIR in case of multiple distfiles) it might be safer to make it a hidden directory.

69

out of curiosity, are there ports with subdirectories in PATCHDIR ? (questioning the -maxdepth)

118

if this is supposed to be a tabulation (can't tell with phabric) maybe it should be escaped.

161–162

maybe rm -rf ${COMMENTS} then mkdir -p ${COMMENTS} ? it feels strange this way around. (same for all other interations of mkdir/rm)

"While since rP400846 no port should ever write any files in WRKDIR and everything should be done in WRKSRC (and other subdirectories in WRKDIR in case of multiple distfiles) it might be safer to make it a hidden directory."

I don't understand this.
${WRKSRC} generally is the vendor distfile expanded.
I definitely don't want to use that area.
Why is $WRKDIR off-limits now? I want to work in there and not vendor area.

"out of curiosity, are there ports with subdirectories in PATCHDIR"

I don't know of any; it's just a correctness thing.

" this is supposed to be a tabulation (can't tell with phabric) maybe it should be escaped."
it is a tab. "\t" didn't work for me but I might have checked before wrapping the whole thing in quotes. It is not wrong now though is it?

"maybe rm -rf ${COMMENTS} then mkdir -p ${COMMENTS} ? it feels strange this way around. (same for all other interations of mkdir/rm)"

if you run makepatch again and again, then it only creates the directory once. I don't think it matters on either order.

Can I get approval for this version? I'd like to commit it today.

The only thing holding it up is this, "While since rP400846 no port should ever write any files in WRKDIR ..."

It's not the port writing to WRKDIR, it's the framework, which is the intended use of WRKDIR. I think the current implementation of ${WRKDIR}/makepatch-tmp is acceptable (there will be no collisions with that directory).

If people agree, I think the change can be improved and people can start using it immediately.

mat added a reviewer: mat.

provided the workarea directory gets hidden.

This revision is now accepted and ready to land.Nov 15 2015, 3:02 PM