Page MenuHomeFreeBSD

WIP: check for REINPLACE_CMD usage that doesn't do anything
ClosedPublic

Authored by swills on Oct 29 2019, 4:12 PM.

Details

Summary

There are many places in ports where REINPLACE_CMD is used but doesn't do anything. It's a frequent source of issues. This adds way to check that the REINPLACE_CMD made a change and complain about it if not.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

swills created this revision.Oct 29 2019, 4:12 PM
swills retitled this revision from check for REINPLACE_CMD usage that doesn't do anything to WIP: check for REINPLACE_CMD usage that doesn't do anything.Oct 29 2019, 4:22 PM
bdrewery added inline comments.
Tools/scripts/sed_checked.sh
8 ↗(On Diff #63759)

[ -f $blah ] doesn't generate output so the redirections aren't needed.

9–16 ↗(On Diff #63759)

You quoted the first if [ -f but then lost quotes everywhere else. It will be significant with filenames with spaces or globbing characters. Unlikely but trivial to avoid the problem.

bdrewery added inline comments.Oct 29 2019, 10:56 PM
Mk/bsd.port.mk
2016 ↗(On Diff #63759)

Is there a review dependency missing here? I'm not sure what FRAMEWORK_REINPLACE_CMD is.

swills updated this revision to Diff 63778.Oct 30 2019, 1:49 AM

Updated WIP

swills marked 2 inline comments as done.Oct 30 2019, 1:51 AM

I had added FRAMEWORK_REINPLACE_CMD as a way to avoid warnings from REINPLACE_CMD but haven't submitted a review for it yet because I'm not sure it's needed if this is going to just be a warning. But if you want, see:

https://reviews.freebsd.org/differential/diff/63739/

Thanks for taking a look, I think the other comments should be addressed. Better, but still needs work.

mat added a subscriber: mat.Nov 5 2019, 11:42 AM
mat added inline comments.
Mk/bsd.port.mk
1621–1622 ↗(On Diff #63778)

Why add WRKDIR and WRKSRC, it is not using it.

2012 ↗(On Diff #63778)

REWARNFILE should be defined once and not have magic values here and there :-)

Tools/scripts/sed_checked.sh
8–9 ↗(On Diff #63778)
if !cmp "${x}" "${x}".bak > /dev/null 2>&1; then

(or maybe without the !, not sure)

swills updated this revision to Diff 64024.Nov 6 2019, 9:30 PM

Address comments from @mat

swills marked an inline comment as done.Nov 6 2019, 9:30 PM
swills marked an inline comment as done.
swills updated this revision to Diff 64063.Nov 7 2019, 9:45 PM

Fix braino

swills marked an inline comment as done.Nov 7 2019, 9:46 PM
swills added a comment.Nov 9 2019, 2:50 PM

In my testing, this finds a large number of errors and of various types. The only issue I've found with it so far is places were we replace /usr/local with LOCALBASE or PREFIX which is also /usr/local, which produces a false alarm from this script. On the other hand, it finds a large number of real problems of various types. One example is this:

https://svnweb.freebsd.org/ports/head/print/alignmargins/files/patch-alignmargins?r1=127541&r2=127691

where the committer accidentally changed LOCALBASE to /usr/local, and no one has noticed for 14 years. Another is sysutils/and which patches a file then calls REINPLACE_CMD on it to replace what it patched into the file. And another is www/amphetadesk which calls REINPLACE_CMD on every file in WRKSRC, including images.

So I do find this to be a really useful check and would like to commit it soon.

mat added inline comments.Nov 13 2019, 2:40 PM
Mk/Scripts/qa.sh
5–6 ↗(On Diff #64063)

Not sure this change is needed. We're only checking for the 3 most important variables. And even then, qa.sh is only run from one place, where those three variables are always defined.

In any way, REWARNFILE is always added to the environment in bsd.port.mk, so I feel this is not needed.

Tools/scripts/sed_checked.sh
6 ↗(On Diff #64063)

cmp as a silent mode:

if cmp -s "${x}" "${x}".bak ; then
swills updated this revision to Diff 64329.Nov 14 2019, 4:38 PM
swills marked an inline comment as done.
tobik added a subscriber: tobik.Dec 5 2019, 4:09 PM
tobik added inline comments.
Tools/scripts/sed_checked.sh
3 ↗(On Diff #64329)

This script always has exit status 0 which means that the blackbox
behavior now differs when DEVELOPER is set and when it is not. If
I introduce a syntax error in a sed expression the build will
continue unpatched anyway when it probably should abort like before.
Maybe add set -e or || exit 1 to the script or handle the error
case in some other way.

8 ↗(On Diff #64329)

Every file flagged here will always be prefixed with ${WRKDIR} which
seems redundant. WRKDIR can also be a pretty long path. Would it
make sense to strip it like what is done in the shebang and symlink
checks in qa.sh?

swills updated this revision to Diff 65425.Dec 9 2019, 3:10 PM
swills marked 2 inline comments as done.

Address comments.

Tools/scripts/sed_checked.sh
3 ↗(On Diff #64329)

Sure, that's fine.

8 ↗(On Diff #64329)

Good idea.

swills marked 3 inline comments as done.Dec 9 2019, 3:12 PM
mat accepted this revision as: portmgr.Jan 3 2020, 10:10 PM
swills updated this revision to Diff 66329.Jan 4 2020, 12:56 AM

Went to commit this and realized I missed a few changes I'd added to avoid issues/false positives from USES, adding those to the diff.

This revision is now accepted and ready to land.Jan 7 2020, 8:54 AM
This revision was automatically updated to reflect the committed changes.