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.
Details
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Mk/bsd.port.mk | ||
---|---|---|
2016 | Is there a review dependency missing here? I'm not sure what FRAMEWORK_REINPLACE_CMD is. |
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.
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:
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.
Mk/Scripts/qa.sh | ||
---|---|---|
5–6 | 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 | cmp as a silent mode: if cmp -s "${x}" "${x}".bak ; then |
Tools/scripts/sed_checked.sh | ||
---|---|---|
4 | This script always has exit status 0 which means that the blackbox | |
9 | Every file flagged here will always be prefixed with ${WRKDIR} which |
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.