BUGFIX: Only call sed_checked.sh if REINPLACE_ARGS hasn't been altered

[grembo]

Summary

NOTE: This is a bugfix to correct broken behaviour, not a new idea or feature.

In r522484[0] a new warning has been introduced to check if
REINPLACE_CMD had any effect. This is accomplished by
calling a script called sed_checked.sh and is only done
in DEVELOPER mode and only if REINPLACE_CMD hasn't been
defined.

Unfortunately, sed_checked.sh assumes that REINPLACE_ARGS
is -i.bak. If it isn't (e.g., -i ""), the working
directory will contain *.bak files while in DEVELOPER mode,
while containing whatever was defined in REINPLACE_ARGS
(no backup files in case of -i "") when not being in
DEVELOPER mode.

This impacts commands like make makeplist and isn't
detected while running in DEVELOPER mode (which includes
running poudriere testport), but will fail outside of
it, e.g., when running poudriere bulk.

This patch fixes this by simply not calling sed_checked.sh
in case REINPLACE_ARGS isn't set to the default "-i.bak".
This seemed to be the most straightforward way to correct this,
I also had other ideas like altering sed_checked.sh to
always use a custom extension like .sedchecked and clean
up after itself).

See also:
https://lists.freebsd.org/pipermail/freebsd-ports/2021-January/119978.html

[0] https://reviews.freebsd.org/D22174

Test Plan

Test building devel/phabricator before and after the patch,
in both DEVELOPER and non-DEVELOPER mode. Do the same for
devel/arcanist (which doesn't alter REINPLACE_ARGS).

[mandree]

Michael, also see https://reviews.freebsd.org/D24240 for an in-depth fix to most things. I'd like to know if with my changed sed_checked.sh, your problem reproduces. (Note that the original change was prior to a relocation from Tools/scripts to Mk/Scripts)

[mat]

I don't think this is a good idea. Having REINPLACE_ARGS overridable was probably a bad idea, and the ability to change -i is also something that needs to go away.

[mandree]

mat, we will be in for major cleanup if the -iWHATEVER option is not overridable, and people will rather reimplement their own REINPLACE_CMD replacements if you mutilate it to the point it can no longer be adapted. My sed_checked.sh that you dismissed calling it names already validates arguments... https://reviews.freebsd.org/D24240 needs a proper technical review and approval, not something polemic or political, to end the stream of other issues with REINPLACE_CMD.

[grembo]

In D27954#624718, @mat wrote:

I don't think this is a good idea. Having REINPLACE_ARGS overridable was probably a bad idea, and the ability to change -i is also something that needs to go away.

@mat Well, why don't we commit this to fix what was actually broken in r522484 (this one-line patch is absolutely non-intrusive and doesn't make anything worse) to stop people from running into the problem now and you come up with and execute a plan to retire overriding REINPLACE_ARGS as a separate effort?

[swills]

I agree with @mat, it's probably better to avoid the problem by not allow overriding REINPLACE_ARGS in the first place. There aren't that many ports that override it, looks to be less than 90, so not that big of a clean up IMHO.

[mat]

In D27954#624728, @grembo wrote:

In D27954#624718, @mat wrote:

I don't think this is a good idea. Having REINPLACE_ARGS overridable was probably a bad idea, and the ability to change -i is also something that needs to go away.

@mat Well, why don't we commit this to fix what was actually broken in r522484 (this one-line patch is absolutely non-intrusive and doesn't make anything worse) to stop people from running into the problem now and you come up with and execute a plan to retire overriding REINPLACE_ARGS as a separate effort?

I am sorry but this is a no. The ports are broken, and the framework is telling you they are, and all you want to do is stop the framework telling you they are broken. If you don't care, just don't build with DEVELOPER set, and everything will be silent again.

[grembo]

I am sorry but this is a no. The ports are broken, and the framework is telling you they are, and all you want to do is stop the framework telling you they are broken. If you don't care, just don't build with DEVELOPER set, and everything will be silent again.

@mat Did you read the summary? The problem is that with DEVELOPER turned on the framework did not tell me that the ports were broken, they only started to fail in bulk mode, which is nasty.

[swills]

It seems to me, and maybe I'm wrong, that this particular case could have been avoided by checking the output of make makeplist more carefully, it's not perfect, never has been and never will be. Am I missing something?

[grembo]

In D27954#624793, @swills wrote:

It seems to me, and maybe I'm wrong, that this particular case could have been avoided by checking the output of make makeplist more carefully, it's not perfect, never has been and never will be. Am I missing something?

This would have avoided installing a couple of .bak files by accident (by the way, pkg-plist is 9000+ lines in this case), which wouldn't have any impact on the package, or its functionality (or even really the size of the package). As this port never created .bak files in the past (due to REINPLACE_ARGS being -i ""), I also didn't specifically look for them. I relied on the framework's QA steps to catch plist problems (the kind that break a build, not the kind that might install an unnecessary 1kb file by accident).

To me, the usual process is:

• Do update (make, make makesum etc.)
• Do a lot of testing if the port actually still works as expected.
• Run make makeplist
• Visually inspect plist and fix the usual suspects (like rc scripts generated from .in files).
• Run various qa targets manually (with DEVELOPER in make.conf).
• Run on multiple poudriere instances using poudriere testport

I can, of course, add poudriere bulk to this procedure, but my basic assumption would be that if something passes all checks in DEVELOPER mode, it should also build outside of DEVELOPER mode.

I don't really understand why you oppose a simple fix that has no negative side effects, makes broken things entering the ports tree less likely, and can be removed easily in case you get rid of REINPLACE_ARGS in the future. I also offered to do something more elaborate (like, still do all the things that sed_checked does, without leaving files behind), but I assumed something very simple and easy to audit would be preferable as a stop-gap solution. Instead I'm told that "all I want to do is the stop the framework telling me they [my port] are broken" and that it's my own fault for relying on the framework/not proof-reading pkg-plist carefully enough.

[swills]

In D27954#624798, @grembo wrote:

In D27954#624793, @swills wrote:

It seems to me, and maybe I'm wrong, that this particular case could have been avoided by checking the output of make makeplist more carefully, it's not perfect, never has been and never will be. Am I missing something?

This would have avoided installing a couple of .bak files by accident (by the way, pkg-plist is 9000+ lines in this case), which wouldn't have any impact on the package, or its functionality (or even really the size of the package). As this port never created .bak files in the past (due to REINPLACE_ARGS being -i ""), I also didn't specifically look for them. I relied on the framework's QA steps to catch plist problems (the kind that breaks a build, not that kind that might install an unnecessary 1kb file by accident).

To me, the usual process is:

• Do update (make, make makesum etc.)
• Run make makeplist
• Do a lot of testing if the port actually still works as expected.
• Visually inspect plist and fix the usual suspects (like rc scripts generated from in files).
• Run various qa targets manually (with DEVELOPER in make.conf).
• Run on multiple poudriere instances using poudriere testport

I can, of course, add poudriere bulk to this procedure, but my basic assumption would be that if something passes all checks in DEVELOPER mode, it should also build outside of DEVELOPER mode.

I don't really understand why you oppose a simple fix that has no negative side effects, makes broken things entering the ports tree less likely, and can be removed easily in case you get rid of REINPLACE_ARGS in the future. I also offered to do something more elaborate (like, still do all the things that sed_checked does, without leaving files behind), but I assumed something very simple and easy to audit would be preferable as a stop-gap solution. Instead I'm told that "all I want to do is the stop the framework telling me they [my port] are broken" and that it's my own fault for relying on the framework/not proof-reading pkg-plist carefully enough.

I'm sorry if you're frustrated, I understand you're trying to make things better.

I don't understand why you say this change makes problems less likely. To me, it's disabling the check for a number of uses, which makes things more error prone. What am I missing?

FWIW, I do think it might be a good idea to add poudriere bulk to your standard process, it's what I do as well, but that's not necessarily related to this change.

[grembo]

In D27954#624840, @swills wrote:

I don't understand why you say this change makes problems less likely. To me, it's disabling the check for a number of uses, which makes things more error prone. What am I missing?

It makes it less likely to check something in that has the wrong pkg-plist (as the working tree differs between DEVELOPER and non-DEVELOPER at the moment if REINPLACE_ARGS is set to something non-standard) or something that builds differently, as the working directory contains a different set of files (even though this might be more of a theoretical problem).

On the mailing list I suggested a more complete solution by running REINPLACE_CMD twice in DEVELOPER mode - once to check if there was a change (e.g., using a custom extension), which would clean up after itself by moving the .bak file back to the original file right afterwards, and then once with REINPLACE_ARGS as specified. This wouldn't be too hard to implement and make sure QA is run (getting both), but still quite elaborate to test and review, hence the simple stop-gap solution in this patch.

Especially given that you seem to want to remove REINPLACE_ARGS (which I don't really object to), any complex solution would be overkill though. I'll certainly remove REINPLACE_ARGS from my ports now anyway, so I (hopefully) won't be affected by any future changes in that area.

FWIW, I do think it might be a good idea to add poudriere bulk to your standard process, it's what I do as well, but that's not necessarily related to this change.

I'll do that for sure, maybe this is also something poudriere could do automatically when running testport (with an option to disable the feature, or a different command). Might be annoying, but would make sure that it's not forgotten - as a user I would prefer that if "testport" works, "bulk" will work as well. But that's for a different day.

[grembo]

@swills p.s. Besides the patch being simple, I figured that not calling sed_checked.sh was in line with what is done in case REINPLACE_CMD has been overridden, as changing REINPLACE_ARGS is also a way to alter REINPLACE_CMD.

I spent enough time on this problem and my efforts to promote a simple fix, therefore I'll abandon the revision. You are aware of the problem now (if you weren't already when D24240 was opened, also, the comment in line 2021 indicates that there have been issues with this feature). It's up to you if you chose to adopt the patch, patch it in a more elaborate way like described above, give @mandree's patch a chance, and/or simply discontinue support for REINPLACE_ARGS.

[mandree]

REINPLACE_CMD or something similar will be needed for machine edits of ports that were not developed by upstream with BSD portability in mind.
Abandoning REINPLACE_CMD altogether will see workarounds filed, and those will very likely escape the QA we have in place.

It is a shame for the same portmgr@ members that have ignored or derided D24240 and surrounding discussions to close their minds for fixing yet another corner case, and stubbornly want to poke their heads through the brick wall with "cleanup" without really looking at matters that are filed and proposed.

[grembo]

@mat @swills It doesn't seem like you got around to resolve this yet, so we're still in a situation where the documented way of using REINPLACE_ARGS will create different workdirs depending if DEVELOPER is on or not.

Are you still planning to make REINPLACE_ARGS non-overridable? Do you need help to do so? (it would only affect a handful of ports and those could easily be changed to use override REINPLACE_CMD for the time being, won't make them any more complex). This would also allow @mandree to close D24240.