reinplace should warn if nothing got replaced

[ryandesign (Ryan Carsten Schmidt)]

What's always bugged me about reinplace is that you get no notification if nothing got replaced, which will probably bite you later. If you use a patchfile to modify a file, and the patch is out of date, the patch fails to apply and the port command exits with an error. This is good; it alerts the port author to the situation and they can fix it by changing or removing the patch. But with reinplace, if the underlying file you're modifying has changed such that the reinplace no longer causes anything to change, you get no warning and the port command proceeds.

I think the correct behavior might ultimately be to error out entirely if a reinplace doesn't change a file, just like we do with failed patches. But as a first step, to get port authors used to this change, it might be friendlier to just print a warning for now.

The attached patch adds a warning if a reinplace doesn't change the underlying file.

[jkh@…]

This behavior should be made conditional. There are legitimate usage cases where a replacement might "fail", and before deciding whether this new behavior should be the default or not it should also be tested out in the ports tree to see how many ports actually break with the new behavior.

[ryandesign (Ryan Carsten Schmidt)]

I agree it should be tested. I've been meaning to reinstall MacPorts in a different prefix, so I may take this opportunity to do that and reinstall all the ports I use and see if my new warning appears anywhere.

[afb@…]

Issuing a warning shouldn't break the port build, even if it could be annoying if wrong.

[ryandesign (Ryan Carsten Schmidt)]

Replying to afb@macports.org:

Issuing a warning shouldn't break the port build, even if it could be annoying if wrong.

Exactly.

So far in my limited tests the warning has been triggered in aquaterm, gd2, libsdl, perl5.8 and perl5.10.

[raimue (Rainer Müller)]

Not sure if it is faster or better than diff -q, but cmp -s $file $tmpfile returns as exit value if two files differ, without any output on stdout (getting rid of >/dev/null).

[ryandesign (Ryan Carsten Schmidt)]

Replying to jkh@apple.com:

This behavior should be made conditional. There are legitimate usage cases where a replacement might "fail", and before deciding whether this new behavior should be the default or not it should also be tested out in the ports tree to see how many ports actually break with the new behavior.

I've found one such possibly legitimate use case in the mysql5 port, which does a reinplace on every manual page and configuration file, not all of which contain the string to be replaced. This causes many warnings to appear.

    # Fix paths in manpages and sample configuration files
    foreach manpage [glob -type f ${destroot}${prefix}/share/man/man\[1-9\]/*] {
        reinplace "s|/etc/my.cnf|${sysconfdir}/my.cnf|g" ${manpage}
    }
    foreach samp_conffile [glob -type f ${destroot}${prefix}/share/${mysql}/mysql/my-*.cnf] {
        reinplace "s|/etc/my.cnf|${sysconfdir}/my.cnf|g" ${samp_conffile}
    }

Replying to raimue@macports.org:

Not sure if it is faster or better than diff -q, but cmp -s $file $tmpfile returns as exit value if two files differ, without any output on stdout (getting rid of >/dev/null).

Thank you, Rainer, I didn't know cmp. That's surely better. I'll attach a new patch.

[ryandesign (Ryan Carsten Schmidt)]

This patch helped me discover that eclipse-ecj32 was broken; see r38136.

[raimue (Rainer Müller)]

Added a -f option

[raimue (Rainer Müller)]

I added reinplace-warning3.diff which includes a -f option to force the replacement and ignore the warning in cases where this is needed.

Now we need to decide if we want an option to ignore the warning or print it always.

[ryandesign (Ryan Carsten Schmidt)]

change the option to -q for "quiet"

[ryandesign (Ryan Carsten Schmidt)]

Maybe there should still be some indication in debug mode that a reinplace didn't do anything, even if the quiet switch is given. What do you think?

[ryandesign (Ryan Carsten Schmidt)]

show warning in debug mode even if quiet switch is used

[ryandesign (Ryan Carsten Schmidt)]

When this ticket is resolved, fix #16384 too.

[ryandesign (Ryan Carsten Schmidt)]

refreshed patch

[tenomoto (Takeshi Enomoto)]

Cc Me!

[tenomoto (Takeshi Enomoto)]

reinplace-warning7.diff is the difference from base_2_1_2.

[cooljeanius (Eric Gallager)]

Cc Me!

[kurthindenburg (Kurt Hindenburg)]

Cc Me!

[kurthindenburg (Kurt Hindenburg)]

updated for current base

[kurthindenburg (Kurt Hindenburg)]

What's the status of this? Is there objections to committing it? The 7.diff had a typo so it didn't work - my patch 8 swapped the "if" as there's no reason to do the compare if we aren't going to print anything.

[larryv (Lawrence Velázquez)]

What happened to the quiet debug message?

[kurthindenburg (Kurt Hindenburg)]

The "-q" still works to not print the warning.

[ryandesign (Ryan Carsten Schmidt)]

We (well, I) seem to have forgotten comment:10.

My plan was to commit just enough of the patch to introduce the -q flag and make it do nothing at all. Then wait until a version of MacPorts is released supporting that do-nothing flag. Then commit the rest of the patch to make reinplace actually print the warning and have -q suppress it. Then we can start adding -q to portfiles and portgroups where needed, without having the currently-released version of MacPorts issue an error about an unknown -q flag.

[kurthindenburg (Kurt Hindenburg)]

just adding the -q part - does nothing else

[kurthindenburg (Kurt Hindenburg)]

Is this latest 9.diff what you meant?

[ryandesign (Ryan Carsten Schmidt)]

Yeah that looks good.

[kurthindenburg (Kurt Hindenburg)]

added r124909 - now we just have to remember to add the rest later.

[kurthindenburg (Kurt Hindenburg)]

2 releases are out since the previous patch - can I commit the rest of this patch?

[raimue (Rainer Müller)]

The code you added in r124909 is only on trunk and targets the next minor release, i.e. the next increment of x in 2.x.y. The releases you mention were made for MacPorts 2.3 from the release_2_3 branch and contained bugfixes only. The new reinplace -q flag was not merged to the release_2_3 branch, nor should it be, as it adds a new feature. At the time of this writing, it will be part of MacPorts 2.4.

[raimue (Rainer Müller)]

Cc Me!

[ryandesign (Ryan Carsten Schmidt)]

MacPorts 2.4.0 was released with the dummy -q flag, so the rest of the code can now be committed for release in 2.4.x or 2.5.0 and this ticket can then be closed.

[Kurt Hindenburg <kurt.hindenburg@…>]

In 8460505f/macports-base:

base: warning if reinplace doesn't change anything

see: #15514

[kurthindenburg (Kurt Hindenburg)]

It appears the 2.4.x commit didn't add to this ticket

base:release-2.4 * 7b288f5 / src/port1.0/portutil.tcl: base: warning if reinplace doesn't change anything ​https://git.io/vDSQf

Changelog commits as well

base:master * 5c57e5b / ChangeLog: Changelog: add entry for reinplace warning ​https://git.io/vDSFn

base:release-2.4 * a15014b / ChangeLog: Changelog: add entry for reinplace warning ​https://git.io/vDSFS

[jmroot (Joshua Root)]

Please set the milestone appropriately when you merge something to a release branch.