[FFe] Upgrade Computer Janitor to 2.0 (dbus edition)

Oh, CJ 2.0 requires python-argparse 1.1 which has already gone through the FFE and MIR process, and is just waiting for a sponsor to upload. mvo has offered to do this.

Also, the gtk ui has been cleaned up so that the unused "Recommended" column is gone. That was a hard-coded no-op previously.

The package has undergone extensive testing by both myself and mvo. CJ 2.0 has more unit tests, and I have user tested the package many times using a Lucid VM. This allowed me to take disk snapshots before any system changing actions, so that I could repeatedly test, revert, test to ensure that everything is working as expected.

Please note that the UI change has already been granted a UI exception.

A +1 from me on this. The old computer-janitor gtk UI has many issues, the threading tends to deadlock and crash randomly. The new dbus based design is much cleaner and much more stable.

- Did that change any of the logic about which packages are considered "cruft"?
- Can you please attach a branch/patch for this, for a quick review?
- Does it change any user-visible strings/break translations?
- How does the frontend behave if the backend crashes? (Throwing DBusExceptions is a common phenomenon with dbus service backed programs, and hard to get right)

This is a pretty bold change for this point in the release cycle, and TBH I'm not really happy about this now. While 2.x might have its known problems, we at least know where we are with it, and it'd seem more prudent to fix a few bugs in that version than changing the entire architecture?

Hi Martin,

On Mar 29, 2010, at 06:37 AM, Martin Pitt wrote:

>- Did that change any of the logic about which packages are considered
>- "cruft"?

I did not. All of that lives inside the computerjanitor Python package, which
actually lives in Update Manager source package. I haven't touched any of
that code for CJ 2.0.

>- Can you please attach a branch/patch for this, for a quick review?

A patch will be huge. The changes have already been merged into the c-j
trunk and are visible here:

http://bazaar.launchpad.net/~computer-janitor-hackers/computer-janitor/trunk/changes/224?start_revid=224

though there have been a few small changes applied after that.

>- Does it change any user-visible strings/break translations?

Yes, it does change some user visible strings.

>- How does the frontend behave if the backend crashes? (Throwing DBusExceptions is a common phenomenon with dbus service backed programs, and hard to get right)

Yes, the front-end currently throws DBusExceptions. What should it do instead?

>This is a pretty bold change for this point in the release cycle, and
>TBH I'm not really happy about this now. While 2.x might have its known
>problems, we at least know where we are with it, and it'd seem more
>prudent to fix a few bugs in that version than changing the entire
>architecture?

I totally understand wanting to be conservative about this application!
However the threading problems are just not feasible to fix separately, which
is why the dbus refactoring was undertaken to make the front-ends single
threaded.

-Barry

Martin asked for an audit of translatable string changes. Here they are to my
best knowledge:

Removed:

* Essential package %s is missing. There may be problems with apt sources.list
  or Packages files may be missing?

  - Because apt verification happens in the dbus service now, there's no good
    way to communicate this to the front-end. What can the user do about it
    anyway?

* Logging to syslog cannot be set up.

  - Gtk front-end now logs exception and exits. General recommendations are
    to not translate log messages since this makes it more difficult to share
    log analysis tools. Log messages are also not generally user friendly or
    read by users.

* Running application, with:
* Pretending to remove cruft: %s

  - Another log message. Also because this is a sentence fragment, it can be
    more difficult to translate.

* Unknown command: %s
* Unknown cruft: %s
* Analyzing system...
* Cleaning Up...
* Removing cruft: %s
* Pretending to post-cleanup: %s
* Post-cleanup: %s
* Could not clean up properly

  - More functionality moved into dbus service. Some of these are log
    messages too.

* computer-janitor must be run as root, sorry.
* Root access required. You must run computer-janitor-gtk as root. Sorry.

  - This is no longer true! :)

Modified:

* computer-janitor cli usage and argument descriptions

  - Well, hard to avoid since usage has changed! Also, usage has been
    expanded to better explain what the tool is for and how it should be
    used.

* Are you sure you want to clean up?

  now: Are you sure you want to clean your system?

  - The old string was uninformative. What is being cleaned up?

* You have chosen to <b>remove %d software packages.</b> Removing packages
  that are still needed can cause errors.

  now: <b>Software packages to remove {packages}</b>.\nRemoving packages that
  are still in use can cause errors.

  - %-substitutions are problematic; how does a user know what's needed?

* Size: %s.
  now: Size: {0}

  - %-substitutions are problematic

New:

* Non-package items to remove: {others}.

  - Helps to know what the other stuff being removed is.

* Processing {0}

  - New progress bar text.

* Landmark package {0.package} is missing
* Landmark package {0.package} is not downloadable
* Duplicate cruft with different cleanup: {0.cruft_name}
* No such cruft: {0.cruft_name}

  - Error messages that actually shouldn't be translated!

BTW, this merge proposal cleans a few of these things up:

https://code.edge.launchpad.net/~barry/computer-janitor/ppa

> >- How does the frontend behave if the backend crashes? (Throwing DBusExceptions is a common phenomenon with dbus service backed programs, and hard to get right)

> Yes, the front-end currently throws DBusExceptions. What should it do instead?

I guess it should handle them gracefully, like restarting the backend or crashing the UI as well, since otherwise the user will be left with a nonfunctioning UI. With such a lot of code changes, some exceptions will be inevitable, I figure.

> String removals

No-brainer, those are ok.

> New:

> * Non-package items to remove: {others}.
> - Helps to know what the other stuff being removed is.

OK.

> * Processing {0}
> - New progress bar text.

OK.

> * Landmark package {0.package} is missing
> * Landmark package {0.package} is not downloadable
> * Duplicate cruft with different cleanup: {0.cruft_name}
> * No such cruft: {0.cruft_name}
> - Error messages that actually shouldn't be translated!

Right, are they marked as such? Does the user actually see them? User-visible text should not use geek terminology like "landmark" (what is that even, in a packaging context?) or "cruft".

> Modified:

CLI help is fine (although it should boil down to a few removed and added strings if you use optparse properly).

Can we please avoid the other string changes? Each of these breaks all the translations which have been done by now. These might be moved to lucid+1?

On Mar 31, 2010, at 03:51 PM, Martin Pitt wrote:

>> Yes, the front-end currently throws DBusExceptions. What should it do
>instead?
>
>I guess it should handle them gracefully, like restarting the backend or
>crashing the UI as well, since otherwise the user will be left with a
>nonfunctioning UI. With such a lot of code changes, some exceptions will
>be inevitable, I figure.

I think dbus should probably be responsible for restarting the backend, but I
can see where the frontends should catch dbus exceptions and exit more
cleanly.

>> * Landmark package {0.package} is missing
>> * Landmark package {0.package} is not downloadable
>> * Duplicate cruft with different cleanup: {0.cruft_name}
>> * No such cruft: {0.cruft_name}
>> - Error messages that actually shouldn't be translated!
>
>Right, are they marked as such? Does the user actually see them? User-
>visible text should not use geek terminology like "landmark" (what is
>that even, in a packaging context?) or "cruft".

They're marked for translation in trunk, but I've removed the markings in
lp:~barry/computer-janitor/ppa -- that branch is currently up for review, but
I am going to modify the branch to include resolution of the above issue, as
well as...

>> Modified:
>
>CLI help is fine (although it should boil down to a few removed and
>added strings if you use optparse properly).

Actually, I'm using argparse 1.1 (which has been given a MIR and an FFe and is
just waiting for cj 2.0 to land), but I will take a look at whether the help
can be adjusted to be more like the previous help.

>Can we please avoid the other string changes? Each of these breaks all
>the translations which have been done by now. These might be moved to
>lucid+1?

I think the other three, while improvements, are not enough to push for in
Lucid, so I'll revert them and create a branch to add them back for lucid+1.

Hi Martin,

lp:~barry/computer-janitor/ppa r233 resolves the outstanding issues I think.

Thanks. This resolved the string changes, but of course the general uncertainty about the completely new architecture remains. Since we are now frozen for beta-2, perhaps you could launch a call for testing on ubuntu-devel@, to collect some more feedback?

On Apr 01, 2010, at 01:01 PM, Martin Pitt wrote:

>Thanks. This resolved the string changes, but of course the general
>uncertainty about the completely new architecture remains. Since we are
>now frozen for beta-2, perhaps you could launch a call for testing on
>ubuntu-devel@, to collect some more feedback?

Good idea Martin, I'll do that.

After further discussion, it's been decided to not land CJ 2.0 in Lucid. I am going to back out the string changes and will get CJ 2.0 landed early in the Maverick cycle. For anybody seeing threading related crashes or freezes, please see my PPA.

This bug was fixed in the package computer-janitor - 2.0.1-0ubuntu1

---------------
computer-janitor (2.0.1-0ubuntu1) maverick; urgency=low

  * Bumping for Ubuntu.

computer-janitor (2.0.1~ppa1) lucid; urgency=low

  * Display dialog in gtk version when another package manager is running,
    preventing package plugin post-cleanup from completing. (LP: #545306)

computer-janitor (2.0~ppa0) lucid; urgency=low

  * Since CJ 2.0 won't make it into Lucid, do Maverick updates now.
    (LP: #552777)
    - Give computer-janitor-gtk --help and --version command line args.
    - Revert translatable strings to post-Lucid improvements
    - Make string substitutions more consistent
    - Update po

computer-janitor (2.0-0ubuntu5) lucid; urgency=low

  * Move some FIXME comments around for less gettext confusion.

computer-janitor (2.0-0ubuntu4) lucid; urgency=low

  * FFe review resolution (LP: #541990)
    - Reverted some strings back to Karmic versions to satisfy FFe for C-J
      2.0 in Lucid. Strings marked with FIXME for updating in Lucid+1.
    - Catch DBusExceptions in both cli and gtk front-ends and print a
      better error message.

computer-janitor (2.0-0ubuntu3) lucid; urgency=low

  * Some cleanup indicated by visual inspection.
    - Move more exception classes to the error.py module.
    - Update docstring.
    - Do not translate logged error messages (can make log analysis
      scripts more difficult).
    - Update copyright years in About dialog.

computer-janitor (2.0-0ubuntu2) lucid; urgency=low

  * Disable the Do... button when there is no cruft to clean up.

computer-janitor (2.0-0ubuntu1) lucid; urgency=low

  * Refactor all package change operations into a dbus service.
    - Switch to using python-argparse
    - Extend functionality of cli version
    - Change format for state.dat ini file
 -- Barry Warsaw <email address hidden> Fri, 21 May 2010 16:55:46 -0400