Merge lp:~lasall/ppa-purge/dryrun into lp:ppa-purge

Proposed by Dominique Lasserre on 2013-03-18
Status: Needs review
Proposed branch: lp:~lasall/ppa-purge/dryrun
Merge into: lp:ppa-purge
Diff against target: 109 lines (+50/-8)
1 file modified
ppa-purge (+50/-8)
To merge this branch: bzr merge lp:~lasall/ppa-purge/dryrun
Reviewer Review Type Date Requested Status
Tormod Volden 2013-03-18 Needs Fixing on 2013-03-21
Review via email: mp+153845@code.launchpad.net

Description of the change

Add dryrun option.

To post a comment you must log in.
Tormod Volden (tormodvolden) wrote :

Thanks, looks good. It would have been even more useful if it could have shown what apt-get would be going to do, but that shouldn't stop this from going in - it is better than nothing.

review: Approve
Dominique Lasserre (lasall) wrote :

Do you mean the --simulate switch of apt-get/aptitude?

Tormod Volden (tormodvolden) wrote :

Yes, but that won't help much if apt-get update hasn't been run with the modified sources list, will it?

Maybe more useful than your current patch, which only helps to verify the PPA name, would be to actually disable the PPA in the sources list (requiring superuser) and run the apt-get install with --simulate, then restore the sources list?

Dominique Lasserre (lasall) wrote :

On 19/03/13 23:09, Tormod Volden wrote:
> Yes, but that won't help much if apt-get update hasn't been run with the modified sources list, will it?
Hm, didn't thought at this.

> Maybe more useful than your current patch, which only helps to verify the PPA name, would be to actually disable the PPA in the sources list (requiring superuser) and run the apt-get install with --simulate, then restore the sources list?
I would rather suggest to use an other switch for that. Because this
operation is much more non-trivial than a "dryrun". If you agree to make
this a new option (and name the switch), I will add the functionality:

 0. Update.
 1. Disable sources but make a backup (where a change is needed),
something like foobar.list.ppa-purge.save (and make sure this file not
already exists).
 2. Update.
 3. --simulate
 4. Restore backup.
 5. Update.

At that point it would be useful to display a message as long the update
is running and remove it with control sequences, e.g.:
  echo -n "Update sources..."; echo -n "\rUpdate finished. \n"

My first motivation for the "dryrun"-switch was to debug the file list
name and see if it's possible to disable other sources than a Launchpad
PPA. (At the moment this is not fully possible - everything works except
disabling the sources entry.)

Tormod Volden (tormodvolden) wrote :

Thanks, that sounds like a nice functionality.

Why do you need step 0?

I don't see the need to remove a message after it has been displayed, only if it would be a pure progress indicator (bars) or something repeated over and over again. This is one advantage with command-line interfaces that you can see afterwards what happened.

I agree it is a little more than a dry-run since it actually updates the package lists. But I am still not convinced we would need your original dry-run. You use it to see if the given repo name will match any apt source entries? As long as there is not many false positives there is not much harm in trying.

I understand you would like to have something symmetric to add-apt-repository. This might be a good idea. In my opinion it would make sense to have an remove-apt-repository that just removes the entries in the sources lists, but with a --revert option to "downgrade" back to package versions available in other sources and a --purge option to remove packages that are not available from the remaining sources. And "ppa-purge" would just be remove-apt-repository --revert --purge.

Dominique Lasserre (lasall) wrote :

On 20/03/13 21:55, Tormod Volden wrote:
> Thanks, that sounds like a nice functionality.
>
> Why do you need step 0?
It is already implemented: line 107

>
> I don't see the need to remove a message after it has been displayed, only if it would be a pure progress indicator (bars) or something repeated over and over again. This is one advantage with command-line interfaces that you can see afterwards what happened.
Ok (I thought at some "." ".." "..." points to indicate something is
running but that is really only some sugar and not important).

>
> I agree it is a little more than a dry-run since it actually updates the package lists. But I am still not convinced we would need your original dry-run. You use it to see if the given repo name will match any apt source entries? As long as there is not many false positives there is not much harm in trying.
Yes that was my usecase. And I didn't want to run an update of my
sources every time (because a single update takes over a minute).

>
> I understand you would like to have something symmetric to add-apt-repository. This might be a good idea. In my opinion it would make sense to have an remove-apt-repository that just removes the entries in the sources lists, but with a --revert option to "downgrade" back to package versions available in other sources and a --purge option to remove packages that are not available from the remaining sources. And "ppa-purge" would just be remove-apt-repository --revert --purge.
Sounds great. I think it would make sense to use python-apt for this
task because it uses the APT API (for either sources entries and package
list files). Imho the right way is to extend the existing
add-apt-repository and eventually use remove-apt-repository as a
shorthand to it. add-apt-repository already resolves its arguments to
valid source entries (either ppa:... or 'deb ...' or 'protocol://...').

Tormod Volden (tormodvolden) wrote :

OK, meanwhile we would be happy to have your --simulate implementation :)

(I won't be reviewing for the next 1-2 weeks, but others might jump in.)

review: Needs Fixing
lp:~lasall/ppa-purge/dryrun updated on 2013-03-22
60. By Dominique Lasserre on 2013-03-22

Simulate removal with --simulate switch.

61. By Dominique Lasserre on 2013-03-22

Fixup variable name.

Dominique Lasserre (lasall) wrote :

I have updated the patch and -n now uses the --simulate switch of apt-get/aptitude.

Dominique Lasserre (lasall) wrote :

Are there any issues with this updated patch?

Tormod Volden (tormodvolden) wrote :

Hi Dominique,
Can you rebase 59-60-61 into one new commit? It is just a bit confusing with commits undoing another.

You have also spelled "disable" wrong at least twice.

Are you serious about checking for 9223372036854775807 backup files? If loads of backup files are already present it will be wrong to add another IMO.

If you are not reusing NUM it is not necessary to unset it.

Tim Lunn (darkxst) wrote :

I guess this branch is dead, but incase anyone else decides to work on it.

You don't need to back up UNIT64_MAX files, just the one. the changes should also be reverted in the trap handler. Alternatively it may be sufficent to just revert the changes i.e remove the "# " in a trap handler and exit failure.

Unmerged revisions

61. By Dominique Lasserre on 2013-03-22

Fixup variable name.

60. By Dominique Lasserre on 2013-03-22

Simulate removal with --simulate switch.

59. By Dominique Lasserre on 2013-03-18

Add dryrun switch "-n" (lp: #802853)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ppa-purge'
2--- ppa-purge 2012-11-21 09:30:49 +0000
3+++ ppa-purge 2013-03-22 17:08:26 +0000
4@@ -8,7 +8,8 @@
5 F_ARCHS=$(dpkg --print-foreign-architectures)
6 PPA_PKGS=$(mktemp)
7 REVERTS=$(mktemp)
8-trap "rm $PPA_PKGS $REVERTS" 0
9+BACKUPLISTS=$(mktemp)
10+trap "rm $PPA_PKGS $REVERTS $BACKUPLISTS" 0
11
12 # Functions to write output nicely.
13 write_msg() {
14@@ -35,6 +36,7 @@
15 echo " -d [distribution] Override the default distribution choice."
16 echo " -y Pass "-y --force-yes" to apt-get or "-y" to aptitude"
17 echo " -i Reverse preference of apt-get upon aptitude."
18+ echo " -n Simulate removal (no changes)."
19 echo " -h Display this help text"
20 echo
21 echo "Example usage commands:"
22@@ -55,13 +57,14 @@
23 }
24
25 # Command line options
26-while getopts "p:s:d:yih\?" opt; do
27+while getopts "p:s:d:yinh\?" opt; do
28 case "$opt" in
29 p ) PPANAME="$OPTARG" ;;
30 s ) PPAHOST="$OPTARG" ;;
31 d ) DIST="$OPTARG" ;;
32 y ) FORCEINSTALL="true" ;;
33 i ) APTALT="true" ;;
34+ n ) SIMULATE="true" ;;
35 h ) usage 0; ;;
36 \?) usage 1; ;;
37 * ) warn "Unknown option '$opt'"; usage 1; ;;
38@@ -82,6 +85,9 @@
39 fi
40 APT=apt-get; APTALT=aptitude
41 fi
42+if [ -n "$SIMULATE" ]; then
43+ APTARG="$APTARG --simulate"
44+fi
45
46 if echo $1 | grep -q "^ppa:"; then
47 PPAOWNER=$(echo $1 | sed "s#^ppa:\(.*\)/\(.*$\)#\1#")
48@@ -156,7 +162,28 @@
49 # Disable PPA from sources.list files
50 for LIST in $(find /etc/apt/ -name "*.list" -exec readlink -f '{}' \;); do
51 if [ -e $LIST ] && grep -q $PPAOWNER/$PPANAME $LIST; then
52- msg "Disabling $PPAOWNER PPA from $LIST"
53+ if [ -z "$SIMULATE" ]; then
54+ msg "Disabling $PPAOWNER PPA from $LIST"
55+ else
56+ NUM=""
57+ if [ -f "${LIST}.ppa-purge.save" ]; then
58+ NUM=1
59+ while [ -f "${LIST}.ppa-purge${NUM}.save" ]; do
60+ # Take care about the very rare case all files
61+ # in range $LIST.ppa-purge{0,ULONG64_MAX}.save
62+ # already exist and avoid endless loop (will
63+ # not try "negative files" though).
64+ if [ $((NUM++)) = 9223372036854775807 ]; then
65+ warn "Could not find place for a backup file."
66+ exit 1
67+ fi
68+ done
69+ fi
70+ msg "Disabe (and backup) $PPAOWNER PPA from $LIST (to ${LIST}.ppa-purge${NUM}.save)"
71+ echo "${LIST}.ppa-purge${NUM}.save $LIST" >> $BACKUPLISTS
72+ cp -- $LIST "${LIST}.ppa-purge${NUM}.save"
73+ unset NUM
74+ fi
75 sed -ri "\:^[^#]+/${PPAOWNER}/${PPANAME}/:s/^deb/# deb/" $LIST
76 fi
77 done
78@@ -170,11 +197,26 @@
79 # from $REINSTALL directly.
80
81 if $APT $APTARG install $REINSTALL; then
82- msg "PPA purged successfully"
83+ STATE=0
84 elif $APTALT $APTARG install $REINSTALL; then
85- msg "PPA purged successfully using $APTALT fallback"
86+ STATE=1
87 else
88- warn "Something went wrong, packages may not have been reverted"
89- exit 1
90-fi
91+ STATE=2
92+fi
93+
94+if [ -n "$SIMULATE" ]; then
95+ msg "Restore disalbed source entries"
96+ while read BACKUP; do
97+ eval mv -- $BACKUP
98+ done < $BACKUPLISTS
99+ msg "Updating packages lists"
100+ $APT update > /dev/null || warn "$APT update failed for some reason"
101+fi
102+
103+case $STATE in
104+ 0 ) msg "PPA purged successfully" ;;
105+ 1 ) msg "PPA purged successfully using $APTALT fallback" ;;
106+ 2 ) warn "Something went wrong, packages may not have been reverted"; exit 1; ;;
107+esac
108+
109 exit 0

Subscribers

People subscribed via source and target branches

to all changes: