Opened 11 years ago

Closed 2 years ago

Last modified 2 years ago

#15514 closed enhancement (fixed)

reinplace should warn if nothing got replaced

Reported by: ryandesign (Ryan Schmidt) Owned by: macports-tickets@…
Priority: Normal Milestone: MacPorts 2.4.1
Component: base Version: 1.6.0
Keywords: Cc: tenomoto (Takeshi Enomoto), cooljeanius (Eric Gallager), kurthindenburg (Kurt Hindenburg), raimue (Rainer Müller)
Port:

Description

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.

Attachments (9)

reinplace-warning.diff (599 bytes) - added by ryandesign (Ryan Schmidt) 11 years ago.
reinplace-warning2.diff (587 bytes) - added by ryandesign (Ryan Schmidt) 11 years ago.
reinplace-warning3.diff (1.3 KB) - added by raimue (Rainer Müller) 11 years ago.
Added a -f option
reinplace-warning4.diff (1.3 KB) - added by ryandesign (Ryan Schmidt) 11 years ago.
change the option to -q for "quiet"
reinplace-warning5.diff (1.4 KB) - added by ryandesign (Ryan Schmidt) 11 years ago.
show warning in debug mode even if quiet switch is used
reinplace-warning6.diff (1.2 KB) - added by ryandesign (Ryan Schmidt) 8 years ago.
refreshed patch
reinplace-warning7.diff (1.3 KB) - added by tenomoto (Takeshi Enomoto) 7 years ago.
reinplace-warning8.diff (1.3 KB) - added by kurthindenburg (Kurt Hindenburg) 5 years ago.
updated for current base
reinplace-warning9.diff (919 bytes) - added by kurthindenburg (Kurt Hindenburg) 5 years ago.
just adding the -q part - does nothing else

Download all attachments as: .zip

Change History (38)

Changed 11 years ago by ryandesign (Ryan Schmidt)

Attachment: reinplace-warning.diff added

comment:1 Changed 11 years ago by ryandesign (Ryan Schmidt)

Milestone: Port EnhancementsMacPorts base enhancements

comment:2 Changed 11 years ago by 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.

comment:3 Changed 11 years ago by ryandesign (Ryan 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.

comment:4 Changed 11 years ago by afb@…

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

comment:5 in reply to:  4 Changed 11 years ago by ryandesign (Ryan 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.

comment:6 Changed 11 years ago by 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).

comment:7 in reply to:  2 Changed 11 years ago by ryandesign (Ryan 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.

Changed 11 years ago by ryandesign (Ryan Schmidt)

Attachment: reinplace-warning2.diff added

comment:8 Changed 11 years ago by ryandesign (Ryan Schmidt)

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

Changed 11 years ago by raimue (Rainer Müller)

Attachment: reinplace-warning3.diff added

Added a -f option

comment:9 Changed 11 years ago by 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.

Changed 11 years ago by ryandesign (Ryan Schmidt)

Attachment: reinplace-warning4.diff added

change the option to -q for "quiet"

comment:10 Changed 11 years ago by ryandesign (Ryan 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?

Changed 11 years ago by ryandesign (Ryan Schmidt)

Attachment: reinplace-warning5.diff added

show warning in debug mode even if quiet switch is used

comment:11 Changed 11 years ago by ryandesign (Ryan Schmidt)

When this ticket is resolved, fix #16384 too.

Changed 8 years ago by ryandesign (Ryan Schmidt)

Attachment: reinplace-warning6.diff added

refreshed patch

comment:12 Changed 7 years ago by tenomoto (Takeshi Enomoto)

Cc: takeshi@… added

Cc Me!

Changed 7 years ago by tenomoto (Takeshi Enomoto)

Attachment: reinplace-warning7.diff added

comment:13 Changed 7 years ago by tenomoto (Takeshi Enomoto)

reinplace-warning7.diff is the difference from base_2_1_2.

comment:14 Changed 5 years ago by cooljeanius (Eric Gallager)

Cc: egall@… added

Cc Me!

comment:15 Changed 5 years ago by kurthindenburg (Kurt Hindenburg)

Cc: khindenburg@… added

Cc Me!

Changed 5 years ago by kurthindenburg (Kurt Hindenburg)

Attachment: reinplace-warning8.diff added

updated for current base

comment:16 Changed 5 years ago by 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.

comment:17 Changed 5 years ago by larryv (Lawrence Velázquez)

What happened to the quiet debug message?

comment:18 Changed 5 years ago by kurthindenburg (Kurt Hindenburg)

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

comment:19 Changed 5 years ago by ryandesign (Ryan 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.

Changed 5 years ago by kurthindenburg (Kurt Hindenburg)

Attachment: reinplace-warning9.diff added

just adding the -q part - does nothing else

comment:20 Changed 5 years ago by kurthindenburg (Kurt Hindenburg)

Is this latest 9.diff what you meant?

comment:21 Changed 5 years ago by ryandesign (Ryan Schmidt)

Yeah that looks good.

comment:22 Changed 5 years ago by kurthindenburg (Kurt Hindenburg)

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

comment:23 Changed 4 years ago by kurthindenburg (Kurt Hindenburg)

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

comment:24 in reply to:  23 Changed 4 years ago by 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.

comment:25 Changed 4 years ago by raimue (Rainer Müller)

Cc: raimue@… added

Cc Me!

comment:26 Changed 2 years ago by ryandesign (Ryan 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.

comment:27 Changed 2 years ago by Kurt Hindenburg <kurt.hindenburg@…>

In 8460505f/macports-base:

base: warning if reinplace doesn't change anything

see: #15514

comment:28 Changed 2 years ago by kurthindenburg (Kurt Hindenburg)

Resolution: fixed
Status: newclosed

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

comment:29 Changed 2 years ago by jmroot (Joshua Root)

Milestone: MacPorts FutureMacPorts 2.4.1

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

Note: See TracTickets for help on using tickets.