Bug 226926 - [PATCH] Tools/scripts/bump-revision.sh could do with a stronger safety belt
Summary: [PATCH] Tools/scripts/bump-revision.sh could do with a stronger safety belt
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Gerald Pfeifer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-25 20:54 UTC by Gerald Pfeifer
Modified: 2021-08-17 10:38 UTC (History)
5 users (show)

See Also:
gerald: maintainer-feedback-


Attachments
Proposed patch (1.20 KB, patch)
2018-03-25 20:54 UTC, Gerald Pfeifer
no flags Details | Diff
Updated patch (taking into account Matthias' preference) (1.21 KB, patch)
2018-04-02 13:57 UTC, Gerald Pfeifer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gerald Pfeifer freebsd_committer 2018-03-25 20:54:50 UTC
Created attachment 191816 [details]
Proposed patch

Before my commit in revision r464215 (cf. bug #226533) 
Tools/scripts/bump-revision.sh would wreak havoc on the
multimedia/avidemux* ports.

This can hardly be blamed on bump-revision.sh, but a safety belt for
this important script would be a good idea.

My attached patch tries to accomplish that, but using `make -V PORTREVISION`
before and after the bumping and raising an error if the latter value is
lower or equal to the former.

How to reproduce:

  % cd $PORTSDIR
  % svn up -r 464036 multimedia/
  % Tools/scripts/bump-revision.sh multimedia/avidemux*

With my patch this prints:

  INFO: multimedia/avidemux PORTREVISION=	9 found, bumping it by 1.
  INFO: multimedia/avidemux-cli PORTREVISION not found, adding PORTREVISION= 1
  ERROR: multimedia/avidemux-cli PORTREVISION went backwards from 5 to 1!
  INFO: multimedia/avidemux-plugins PORTREVISION not found, adding PORTREVISION= 1 
  ERROR: multimedia/avidemux-plugins PORTREVISION went backwards from 5 to 1!
  INFO: multimedia/avidemux-qt4 PORTREVISION not found, adding PORTREVISION= 1
  ERROR: multimedia/avidemux-qt4 PORTREVISION went backwards from 5 to 1!

The beauty of my approach is that it uses a different approach than simple
text search, and the one that the ports framework actually uses when it runs.


Happy to tweak that patch, otherwise also happy to commit this if you
approve.
Comment 1 Matthias Andree freebsd_committer 2018-03-25 22:27:08 UTC
Since Gerald pointed me to the PR, I take the liberty to add some unsolicited feedback, I hope he does not mind.

- personally, I prefer var="$(some command)" over the backtick notation, because it resolves doubts about quoting

- I suggest to use "pkg version -t pre post" as a comparison, rather than [ ... -le ... ] (aka. test ... -le ...) because I *believe* (without testing) that the existing version would not support lexicographical wraps when numbers get longer, for instance, when bumping from PORTREVISION=9 to ...=10 because 10 sorts before 9 if you sort them as string.
Comment 2 Matthias Andree freebsd_committer 2018-03-25 22:28:26 UTC
(In reply to Matthias Andree from comment #1)
I need to revise that, since we're only comparing the PORTREVISION, so I take the 2nd part back. Sorry for the noise. The first part stands.
Comment 3 Mathieu Arnold freebsd_committer 2018-03-27 08:04:47 UTC
I disagree with the "no safety belt" thing, it is not like it bumps everything and commits it :-)
Comment 4 Thomas Zander freebsd_committer 2018-04-01 15:13:41 UTC
Wrong auto-assignment. multimedia@ is not the maintainer of this script :-)

But I like the idea, Gerald! This would be a significant boost to the tool's usefulness.
Comment 5 Gerald Pfeifer freebsd_committer 2018-04-02 13:57:42 UTC
Created attachment 192106 [details]
Updated patch (taking into account Matthias' preference)

(In reply to Matthias Andree from comment #1)
> - personally, I prefer var="$(some command)" over the backtick notation, 
> because it resolves doubts about quoting

Happy to make this change, updated patch attached.
Comment 6 Gerald Pfeifer freebsd_committer 2018-04-02 14:02:26 UTC
(In reply to Mathieu Arnold from comment #3)
> I disagree with the "no safety belt" thing, it is not like it bumps
> everything and commits it :-)

Fair enough.  Let me adjust the summary a bit. ;-)

Any other comment about the approach or actual patch, Mathieu?


(In reply to Thomas Zander from comment #4)
> But I like the idea, Gerald! This would be a significant boost to the
> tool's usefulness.

Happy you like it, Thomas! :-)  It surely would have avoided a number
of challenges for me bumping PORTREVISIONS over the years.
Comment 7 Gerald Pfeifer freebsd_committer 2018-07-29 10:16:12 UTC
Bartek, any thoughts on this?

I'm going to give this another real world exercise as I update 
GCC_DEFAULT from 6 to 7 later today (hopefully) or tomorrow.
Comment 8 commit-hook freebsd_committer 2018-08-19 09:18:05 UTC
A commit references this bug:

Author: gerald
Date: Sun Aug 19 09:17:57 UTC 2018
New revision: 477561
URL: https://svnweb.freebsd.org/changeset/ports/477561

Log:
  Add a stronger safety belt to this script by comparing the actual
  PORTREVISION (using the Ports Collection framework, not just looking at
  one Makefile individually at a time) before and after the bump.  If the
  version after the bump isn't actually increased, flag that as an error.

  As an example, before revision r464215 (cf. bug #226533) this script
  would have wreaked wreak havoc on the multimedia/avidemux* ports.
  This hardly can be blamed on bump-revision.sh, but with the additional
  safety belt it does now detect such cases.

  How to reproduce:

    % cd $PORTSDIR
    % svn up -r 464036 multimedia/
    % Tools/scripts/bump-revision.sh multimedia/avidemux*

  With this patch we print:

    INFO: multimedia/avidemux PORTREVISION=	9 found, bumping it by 1.
    INFO: multimedia/avidemux-cli PORTREVISION not found, adding PORTREVISION= 1
    ERROR: multimedia/avidemux-cli PORTREVISION went backwards from 5 to 1!
    INFO: multimedia/avidemux-plugins PORTREVISION not found, adding PORTREVISION= 1
    ERROR: multimedia/avidemux-plugins PORTREVISION went backwards from 5 to 1!
    INFO: multimedia/avidemux-qt4 PORTREVISION not found, adding PORTREVISION= 1
    ERROR: multimedia/avidemux-qt4 PORTREVISION went backwards from 5 to 1!

  The beauty of this approach is that it goes beyond a simple text search,
  and leverages what the ports framework itself does.

  PR:		226926, 226533
  Approved by:	maintainer timeout (20+ weeks)
  Reviewed by:	mandree, riggs

Changes:
  head/Tools/scripts/bump-revision.sh
Comment 9 Gerald Pfeifer freebsd_committer 2018-08-19 09:19:58 UTC
This ran into a maintainer timeout, and given that others indicated
this will be useful (and it passed my run to bump for the latest
GCC_DEFAULT update), I went ahead and commit this.
Comment 10 commit-hook freebsd_committer 2018-08-20 00:40:16 UTC
A commit references this bug:

Author: gerald
Date: Mon Aug 20 00:39:33 UTC 2018
New revision: 477615
URL: https://svnweb.freebsd.org/changeset/ports/477615

Log:
  Use 'make -C' instead of saving the current working directory and then
  restoring that in the new code to add a safety belt that came in via
  revision 477561.  This is quite a bit simpler and shorter.

  Reported by:	adamw
  PR:		226926

Changes:
  head/Tools/scripts/bump-revision.sh
Comment 11 commit-hook freebsd_committer 2021-08-17 10:38:36 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=153dcd1ca8065d973d9d7ae5cc0d39612e4af8d9

commit 153dcd1ca8065d973d9d7ae5cc0d39612e4af8d9
Author:     Daniel Engberg <diizzy@FreeBSD.org>
AuthorDate: 2021-08-17 10:15:40 +0000
Commit:     Daniel Engberg <diizzy@FreeBSD.org>
CommitDate: 2021-08-17 10:37:17 +0000

    print/tex-basic-engines: Update maintainer from freebsd-tex@ to tex@

    PR:             226926
    Approved by:    tcberner (mentor)

 print/tex-basic-engines/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)