New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite auto-remove to always auto-remove all orphaned dependencies #6

Merged
merged 5 commits into from May 26, 2015

Conversation

Projects
None yet
5 participants
@GhostLyrics
Contributor

GhostLyrics commented May 5, 2015

Let me know if this works for you or if I should try to rebuild it into a new parameter (though I’d honestly prefer not to do that).

@watercrossing

This comment has been minimized.

Show comment
Hide comment
@watercrossing

watercrossing May 12, 2015

This works perfectly on 14.04. Thanks!

watercrossing commented May 12, 2015

This works perfectly on 14.04. Thanks!

@Burstaholic

This comment has been minimized.

Show comment
Hide comment
@Burstaholic

Burstaholic May 13, 2015

👍 This is badly needed!

Burstaholic commented May 13, 2015

👍 This is badly needed!

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Hey, thanks for the branch and sorry for my slow reply. This branch looks great. I'm sitting on the fence a bit about adding a new option or not. Technically this is a behaviour change but given that a) the old feature apparently never worked reliable b) the wording is already talking about removing dependencies I think its fine to merge it. The thing I would like to see is some tests and extraction of the common code, I will look into this now.

Owner

mvo5 commented May 26, 2015

Hey, thanks for the branch and sorry for my slow reply. This branch looks great. I'm sitting on the fence a bit about adding a new option or not. Technically this is a behaviour change but given that a) the old feature apparently never worked reliable b) the wording is already talking about removing dependencies I think its fine to merge it. The thing I would like to see is some tests and extraction of the common code, I will look into this now.

GhostLyrics added some commits May 26, 2015

simplify early exit check
Since all conditions are combined with 'AND' the brackets can be
removed without harm. The rewrite also makes pep8 a lot happier.
@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics May 26, 2015

Contributor

I just cleaned up the code to remove all the pep8 warnings. Sorry for that, don't have python3 on my Mac. Please re-check the early-exit condition in case I was too eager to simplify.

Do you have a primer for starting with the tests?

Contributor

GhostLyrics commented May 26, 2015

I just cleaned up the code to remove all the pep8 warnings. Sorry for that, don't have python3 on my Mac. Please re-check the early-exit condition in case I was too eager to simplify.

Do you have a primer for starting with the tests?

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Hey Alexander! Looks like we may have a mid-air collision here :) I pushed what I had in mind for the cleanup here: https://github.com/mvo5/unattended-upgrades/tree/GhostLyrics-debian/jessie, its just extracting some common code. If you could merge that into your branch, that would rock. As for the tests, I have a look as the code is not that great when it comes to testability :/

Owner

mvo5 commented May 26, 2015

Hey Alexander! Looks like we may have a mid-air collision here :) I pushed what I had in mind for the cleanup here: https://github.com/mvo5/unattended-upgrades/tree/GhostLyrics-debian/jessie, its just extracting some common code. If you could merge that into your branch, that would rock. As for the tests, I have a look as the code is not that great when it comes to testability :/

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Oh and thanks for the pep8 cleanup!

Owner

mvo5 commented May 26, 2015

Oh and thanks for the pep8 cleanup!

@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics May 26, 2015

Contributor

@mvo5 not sure if that was by accident, but in GhostLyrics-debian/jessie you removed the possibility to test removal via --dry-run as far as I can see.

Contributor

GhostLyrics commented May 26, 2015

@mvo5 not sure if that was by accident, but in GhostLyrics-debian/jessie you removed the possibility to test removal via --dry-run as far as I can see.

@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

Hi,

I'd like to test this, but I don't have any auto-removable package on my systems at the moment and I don't know how to reliably recreate this condition.

I tried this

  • deborphan
  • pickup some package I don't really need
  • aptitude markauto $(package) : this either remove package immediately or doesn't do anything
    I have to use the TUI to mark it auto
  • then neither apt-get autoremove nor U-U detects this package as autoremovable
Contributor

a-detiste commented May 26, 2015

Hi,

I'd like to test this, but I don't have any auto-removable package on my systems at the moment and I don't know how to reliably recreate this condition.

I tried this

  • deborphan
  • pickup some package I don't really need
  • aptitude markauto $(package) : this either remove package immediately or doesn't do anything
    I have to use the TUI to mark it auto
  • then neither apt-get autoremove nor U-U detects this package as autoremovable
@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

(where faking upgrades is a simple matter of pinning random things from sid/experimental)

Contributor

a-detiste commented May 26, 2015

(where faking upgrades is a simple matter of pinning random things from sid/experimental)

@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics May 26, 2015

Contributor

I like to test in a VM with something along the lines of: sudo apt-get install YOURPACKAGE (e.g. nodejs) && sudo apt-get remove YOURPACKAGE

test@ubuntu-vm:~$ sudo apt-get install nodejs
[sudo] password for alex:
…
The following extra packages will be installed:
  libc-ares2 libv8-3.14.5
The following NEW packages will be installed:
  libc-ares2 libv8-3.14.5 nodejs
0 upgraded, 3 newly installed, 0 to remove and 17 not upgraded.
…
Do you want to continue? [Y/n]
…
test@ubuntu-vm:~$ sudo apt-get remove nodejs
…
The following packages were automatically installed and are no longer required:
  libc-ares2 libv8-3.14.5
Use 'apt-get autoremove' to remove them.
The following packages will be REMOVED:
  nodejs
0 upgraded, 0 newly installed, 1 to remove and 17 not upgraded.
…

The important part here is:

The following packages were automatically installed and are no longer required:
  libc-ares2 libv8-3.14.5
Use 'apt-get autoremove' to remove them.
Contributor

GhostLyrics commented May 26, 2015

I like to test in a VM with something along the lines of: sudo apt-get install YOURPACKAGE (e.g. nodejs) && sudo apt-get remove YOURPACKAGE

test@ubuntu-vm:~$ sudo apt-get install nodejs
[sudo] password for alex:
…
The following extra packages will be installed:
  libc-ares2 libv8-3.14.5
The following NEW packages will be installed:
  libc-ares2 libv8-3.14.5 nodejs
0 upgraded, 3 newly installed, 0 to remove and 17 not upgraded.
…
Do you want to continue? [Y/n]
…
test@ubuntu-vm:~$ sudo apt-get remove nodejs
…
The following packages were automatically installed and are no longer required:
  libc-ares2 libv8-3.14.5
Use 'apt-get autoremove' to remove them.
The following packages will be REMOVED:
  nodejs
0 upgraded, 0 newly installed, 1 to remove and 17 not upgraded.
…

The important part here is:

The following packages were automatically installed and are no longer required:
  libc-ares2 libv8-3.14.5
Use 'apt-get autoremove' to remove them.
@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

I added a test in 0ece9e4 that simulates the dependency removal.

Owner

mvo5 commented May 26, 2015

I added a test in 0ece9e4 that simulates the dependency removal.

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

If you merge my changes I'm happy to merge your branch (this way its probably easiest). As for the dry-run option. Its fine to call cache_commit() with dry-run, it will not really do anything expect for printing the dpkg commands it would run. So unless I miss someting (quite possible :) my removal of the "if options.dry_run:" should not have done any harm. If I did miss something, please help me to understand what!

Owner

mvo5 commented May 26, 2015

If you merge my changes I'm happy to merge your branch (this way its probably easiest). As for the dry-run option. Its fine to call cache_commit() with dry-run, it will not really do anything expect for printing the dpkg commands it would run. So unless I miss someting (quite possible :) my removal of the "if options.dry_run:" should not have done any harm. If I did miss something, please help me to understand what!

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Hey a-detiste , another way of testing this is: "sudo apt-get install 2vcard; sudo apt-mark auto 2vcard; sudo ./unattended-upgrades -v" (with auto-removal enabled). This should remove 2vcard again.

Owner

mvo5 commented May 26, 2015

Hey a-detiste , another way of testing this is: "sudo apt-get install 2vcard; sudo apt-mark auto 2vcard; sudo ./unattended-upgrades -v" (with auto-removal enabled). This should remove 2vcard again.

@mvo5 mvo5 merged commit 688698f into mvo5:debian/sid May 26, 2015

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Thanks again, I resolved the conflicts and merged into the debian/sid branch (which is the master branch for unattended-upgrades).

Owner

mvo5 commented May 26, 2015

Thanks again, I resolved the conflicts and merged into the debian/sid branch (which is the master branch for unattended-upgrades).

@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

# exit if there is nothing to do and nothing to report

With the recent changes, UU now sends empty mails when it has nothing to do.

Contributor

a-detiste commented May 26, 2015

# exit if there is nothing to do and nothing to report

With the recent changes, UU now sends empty mails when it has nothing to do.

@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics May 26, 2015

Contributor

@a-detiste do you happen to have missed the following?

// Set this value to "true" to get emails only on errors. Default
// is to always send a mail if Unattended-Upgrade::Mail is set
//Unattended-Upgrade::MailOnlyOnError "true";
Contributor

GhostLyrics commented May 26, 2015

@a-detiste do you happen to have missed the following?

// Set this value to "true" to get emails only on errors. Default
// is to always send a mail if Unattended-Upgrade::Mail is set
//Unattended-Upgrade::MailOnlyOnError "true";
@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

Even with Unattended-Upgrade::MailOnlyOnError "false";, no mail is sent when UU has nothing to do.

Contributor

a-detiste commented May 26, 2015

Even with Unattended-Upgrade::MailOnlyOnError "false";, no mail is sent when UU has nothing to do.

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

That might be my changes, I will see how/why I broke this.

Owner

mvo5 commented May 26, 2015

That might be my changes, I will see how/why I broke this.

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

@a-detiste this 782f9fb should fix the mail issue you see, the problem was that the commit() of a empty change in auto-remove triggers a mail.

Owner

mvo5 commented May 26, 2015

@a-detiste this 782f9fb should fix the mail issue you see, the problem was that the commit() of a empty change in auto-remove triggers a mail.

@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

And thanks for the testing, please let us know if there is more :)

Owner

mvo5 commented May 26, 2015

And thanks for the testing, please let us know if there is more :)

@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

I got this

python3 test_remove_unused.py
No such file or directory: '/home/pi/unattended-upgrades/test/root.unused-deps/var/lib/dpkg/updates
Contributor

a-detiste commented May 26, 2015

I got this

python3 test_remove_unused.py
No such file or directory: '/home/pi/unattended-upgrades/test/root.unused-deps/var/lib/dpkg/updates
@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Thanks! Silly me, I keep forgeting that git does not track dirs, I added a
empty placeholder now.

On Tue, May 26, 2015 at 2:28 PM, Alexandre Detiste <notifications@github.com

wrote:

I got this

python3 test_remove_unused.py
No such file or directory: '/home/pi/unattended-upgrades/test/root.unused-deps/var/lib/dpkg/updates


Reply to this email directly or view it on GitHub
#6 (comment)
.

Owner

mvo5 commented May 26, 2015

Thanks! Silly me, I keep forgeting that git does not track dirs, I added a
empty placeholder now.

On Tue, May 26, 2015 at 2:28 PM, Alexandre Detiste <notifications@github.com

wrote:

I got this

python3 test_remove_unused.py
No such file or directory: '/home/pi/unattended-upgrades/test/root.unused-deps/var/lib/dpkg/updates


Reply to this email directly or view it on GitHub
#6 (comment)
.

@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

I'm still getting the "empty" mails.

Contributor

a-detiste commented May 26, 2015

I'm still getting the "empty" mails.

@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 26, 2015

Contributor

Now I've got this ;-)

python3 test_remove_unused.py                                                                   
An error occurred: '404  Not Found [IP : 91.189.91.24 80]'                                                                                  
The URI 'http://archive.ubuntu.com/ubuntu/test-package_2.0_all.deb' failed to download, aborting
Contributor

a-detiste commented May 26, 2015

Now I've got this ;-)

python3 test_remove_unused.py                                                                   
An error occurred: '404  Not Found [IP : 91.189.91.24 80]'                                                                                  
The URI 'http://archive.ubuntu.com/ubuntu/test-package_2.0_all.deb' failed to download, aborting
@mvo5

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 26, 2015

Owner

Thanks! The test should be fixed now and I also changed the code a bit so
that the removal of unused dependencies is only done if there is a upgrade.
The reason for the empty mails was that the previous check if there is
anythig to do was changed and u-u always acts and sends mail when the
option "remove-unused-dependencies" is set.

On Tue, May 26, 2015 at 3:13 PM, Alexandre Detiste <notifications@github.com

wrote:

Now I've got this ;-)

python3 test_remove_unused.py
An error occurred: '404 Not Found [IP : 91.189.91.24 80]'
The URI 'http://archive.ubuntu.com/ubuntu/test-package_2.0_all.deb' failed to download, aborting


Reply to this email directly or view it on GitHub
#6 (comment)
.

Owner

mvo5 commented May 26, 2015

Thanks! The test should be fixed now and I also changed the code a bit so
that the removal of unused dependencies is only done if there is a upgrade.
The reason for the empty mails was that the previous check if there is
anythig to do was changed and u-u always acts and sends mail when the
option "remove-unused-dependencies" is set.

On Tue, May 26, 2015 at 3:13 PM, Alexandre Detiste <notifications@github.com

wrote:

Now I've got this ;-)

python3 test_remove_unused.py
An error occurred: '404 Not Found [IP : 91.189.91.24 80]'
The URI 'http://archive.ubuntu.com/ubuntu/test-package_2.0_all.deb' failed to download, aborting


Reply to this email directly or view it on GitHub
#6 (comment)
.

@GhostLyrics GhostLyrics deleted the GhostLyrics:debian/jessie branch May 26, 2015

@a-detiste

This comment has been minimized.

Show comment
Hide comment
@a-detiste

a-detiste May 27, 2015

Contributor

@GhostLyrics @mvo5

Hi,

I had this message pop up and I didn't understood what it meant & I had to read the code.
Maybe rephrase that as "Packages were successfully auto-removed" or something.

Contributor

a-detiste commented on unattended-upgrade in ada2a0d May 27, 2015

@GhostLyrics @mvo5

Hi,

I had this message pop up and I didn't understood what it meant & I had to read the code.
Maybe rephrase that as "Packages were successfully auto-removed" or something.

This comment has been minimized.

Show comment
Hide comment
@mvo5

mvo5 May 28, 2015

Owner

Thanks, I changed the message in git now.

Owner

mvo5 replied May 28, 2015

Thanks, I changed the message in git now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment