Bug 192706

Summary: ports/Tools/scripts/bump_revision.pl overhaul, includes limiting to direct dependencies
Product: Ports & Packages Reporter: Matthias Andree <mandree>
Component: Ports FrameworkAssignee: Matthias Andree <mandree>
Status: Closed FIXED    
Severity: Affects Some People CC: emaste, gerald, pi, rakuco
Priority: ---    
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
revised bump_revision.pl
none
updates version for modern perl, deletes variable named ENV to avoid shell abuse
none
overhauled version heeding Gerald's comments none

Description Matthias Andree freebsd_committer 2014-08-16 15:20:12 UTC
Created attachment 145874 [details]
revised bump_revision.pl

I am attaching a rewritten bump_revision.pl script with these major changes:

- possible to give multiple ports on the command line, for instance, ilmbase and OpenEXR, so that all portrevisions of ports that depend on either will be bumped
- a new method (-l option) to limit bumping revisions of ports with direct dependencies (slow, uses make -C cat/port -V LIB_DEPENDS -V RUN_DEPENDS on each of the candidates),
- better hunt for INDEX files (looks at INDEX-10 or thereabouts)
- updated for SVN
- robustness fixes and internal processing fixes, fewer loops where we can map.
Comment 1 Gerald Pfeifer freebsd_committer 2014-08-30 21:03:47 UTC
First of all I apologize for this taking two weeks for me getting
back to.

(It's funny, I've been the only one making improvement to this for
a couple of years, essentially whenever I used it, so I ended up
inheriting it, and now there is wider interest it seems. ;-)

I would have preferred a few independent patches, which are easier
to review, but let me give it a try in one piece:

- Can you please add a note on this adding -T to the options for Perl to
  the commit message?

- In the initial comment, I don't think we have to mention that this is
  Perl script.  For the user that should not matter.

- Why does it "help with bumping" as opposed to just "bump"?

- I'm not convinced /usr/ports is what most people using this script
  will be using for their work; how about saying "such as"?

- I am not necessarily in agreement setting $ENV{'PATH'} as such.  Can
  you please revert this and we can discuss separately from the other
  changes?

- Shouldn't we just default to one of -l and -g, and I guess -g since
  that's currently the case?

- Can you please add a comment above "$| = 1;" for those not too familiar
  with Perl?

- The

     print "$TMPDIR/.svn exists, do you want to use the -n option? Press Ctrl-C to abort.\n";
     sleep 5;

  part feels interesting.  I don't think we should make the result of 
  running this script dependent on whether someone got distracted or
  was looking at her screen.

- # If specified as portname, check all indexes for existence or duplicates
 
  There is only one INDEX in use, so this is a bit confusing.  I assume
  you mean "check the entire INDEX" or so?

- Instead of using ".bump_rev_tmpdir.", how about using the name of the
  script?

- Sometimes it reads ".... Aborting", sometimes "..., aborting".  I suggest
  to always go for the former.

- Should direct_dependence by called direct_dependency?

I assume you see why I would have preferred three, four smaller patches
which I believe would have been less effort overall for both of us?  

Anyway, there is a bit more, but we can tackle that later.  If you consider
the input above, I'm fine with these changes.  And I do trust your Perl skills
which clearly are far superior to mine :-), so did not review the more
intricate bits in detail.
Comment 2 Matthias Andree freebsd_committer 2019-07-09 21:59:19 UTC
Created attachment 205635 [details]
updates version for modern perl, deletes variable named ENV to avoid shell abuse

This revised version adds a "delete $ENV{'ENV'};" so as to avoid a /bin/sh from the Perl system function from pulling in dangerous goods, and to satisfy stricter requirements of Perl's.

Gerald, if you want to hand over maintainership to me, feel free to do so, and commit this new revised revision (file date 2018-09-09) with a changed MAINTAINER= line and let me know.  Note the file name is changed, you need to "mv" it into place before committing. 

Further considerations behind the scenes by e-mail.

We may want to give fellow contributors a short help text discussing advantages of the .sh vs. the .pl version.
Comment 3 Matthias Andree freebsd_committer 2019-07-09 22:04:37 UTC
Comment on attachment 205635 [details]
updates version for modern perl, deletes variable named ENV to avoid shell abuse

Uh, I haven't addressed Gerald's concerns from the original yet. Withdrawing approval request.
Comment 4 Matthias Andree freebsd_committer 2019-07-09 22:27:40 UTC
Created attachment 205636 [details]
overhauled version heeding Gerald's comments

This version addresses Gerald's comments, except the $ENV{PATH} override. We need to set a defined PATH so we don't inherit shit from the environment, and it's technically a requirement for running with strict taint checks (perl -T, see man perlrun and man perlsec).

Further improvement might be through Getopt::Long and long options and better documentation.
Comment 5 Gerald Pfeifer freebsd_committer 2019-07-11 18:35:11 UTC
This is a nice revamp, thank you!

There is one recommendation I have, and perhaps an idea:

Last year I added a safety net to the bump-revision.sh script, that
works as follows:

        # See what the port thinks its PORTREVISION is and save that.
        pre=$(make -C "$1" -V PORTREVISION)
         :
           # See what the port now has for PORTREVISION.
            post=$(make -C "$1" -V PORTREVISION)

            if [ "$post" -le "$pre" ]; then
                printc "ERROR: $1 PORTREVISION went backwards from $pre to $post
!" "red"
            fi

That is, use the actual Ports machinery to determine the actual PORTREVISION
and ensure that does not go backwards.  This can happen when there is no
PORTREVISION set in a Makefile, we add one, but it actually is set somewhere
else via .include.

See `svn log -c477561` for more details.


Now the idea: bump-revision.sh is very simplistic compared to this script.
Perhaps leave the logic and intelligence to this script, and leverage the
simple bump-revision.sh script for the actual bumping?
Comment 6 commit-hook freebsd_committer 2019-07-12 12:20:46 UTC
A commit references this bug:

Author: gerald
Date: Fri Jul 12 12:20:34 UTC 2019
New revision: 506455
URL: https://svnweb.freebsd.org/changeset/ports/506455

Log:
  Transfer maintainership of this script to mandree@ based on conversations
  we've been having after I was planning to EOL it last year and a nice
  revamp of this script he has prepared.

  Thank you for taking this over, and good luck, Matthias!

  PR:		192706

Changes:
  head/Tools/scripts/bump_revision.pl
Comment 7 commit-hook freebsd_committer 2019-07-12 16:08:49 UTC
A commit references this bug:

Author: mandree
Date: Fri Jul 12 16:07:45 UTC 2019
New revision: 506466
URL: https://svnweb.freebsd.org/changeset/ports/506466

Log:
  Update bump_revision.pl.

  Key features:
  - updated for SVN (the old version expected CVS)
  - shallow mode to only bump direct dependencies (option -l)

  PR:		192706
  Submitted by:	mandree@ (new maintainer)

Changes:
  head/Tools/scripts/bump_revision.pl
Comment 8 Matthias Andree freebsd_committer 2019-07-12 16:09:05 UTC
Thanks, Gerald.