Merge ~fourdollars/ubiquity:master into ubiquity:master

Proposed by Shih-Yuan Lee
Status: Merged
Approved by: Jean-Baptiste Lallement
Approved revision: e7257071986c7c2631c1c69673a156e62546d243
Merged at revision: c7e2ec1447d2121935fcf79b8a63d49bf0c7413e
Proposed branch: ~fourdollars/ubiquity:master
Merge into: ubiquity:master
Diff against target: 95 lines (+41/-28)
1 file modified
ubiquity/install_misc.py (+41/-28)
Reviewer Review Type Date Requested Status
Jean-Baptiste Lallement Approve
Iain Lane (community) Approve
Julian Andres Klode (community) Approve
Review via email: mp+395438@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
You-Sheng Yang (vicamo) :
Revision history for this message
Shih-Yuan Lee (fourdollars) :
Revision history for this message
You-Sheng Yang (vicamo) :
Revision history for this message
Shih-Yuan Lee (fourdollars) :
Revision history for this message
You-Sheng Yang (vicamo) :
Revision history for this message
Shih-Yuan Lee (fourdollars) :
Revision history for this message
You-Sheng Yang (vicamo) :
Revision history for this message
Shih-Yuan Lee (fourdollars) :
Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

We encountered another similar problem on a private bug LP: #1908186 on bionic.
The local archives didn't include the new mokutil however it included the new shim-signed that depends on the new mokutil.
Ubiquity just silently removed shim-signed without any exception.
If Ubiquity can raise this exception, we can save many human resources and a lot of time for debug.

Revision history for this message
Julian Andres Klode (juliank) wrote :

So what actually should happen is that all packages are marked with auto_fix=False, auto_inst=False first, and then we run the same loop with auto_fix=False, auto_inst=True; and finally it should create a apt.ProblemResolver(cache) and call resolve() on that.

Doing it otherwise is problematic, and we see that broken code pattern in a lot of python-apt code.

Revision history for this message
Julian Andres Klode (juliank) wrote :

As in this is what apt does, and this is what I want to migrate release upgrader too as well.

I'd prefer not inventing new solvers like this.

Revision history for this message
Iain Lane (laney) wrote :

@fourdollars, can you update this following Julian's feedback please? ^

review: Needs Fixing
Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

I was busy on the OEM metapackages of kernel migrations from 5.6 to 5.8.
I will deal with this next week.

Revision history for this message
Iain Lane (laney) wrote :

On Fri, Jan 15, 2021 at 11:23:29AM -0000, Shih-Yuan Lee wrote:
> I was busy on the OEM metapackages of kernel migrations from 5.6 to 5.8.
> I will deal with this next week.

Sure, no worries!

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

@julian

Could you help to check if the 9316c5e commit meets your expectation?

But I still see cache._depcache.broken_count > 0 to raise InstallStepError.

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :
Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

I can install ibus by `apt install ibus` manually and it will install ibus, ibus-data, gir1.2-ibus-1.0 and python3-ibus-1.0.
However I can not find python3-ibus-1.0 in https://paste.ubuntu.com/p/VyYB4xDyb6/. Wired.

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

Err. I mean weird.

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

https://paste.ubuntu.com/p/mdnqSvtwxk/ is the log of c9d32cb on the top of ubiquity 20.04.15.2 on focal, and it seems to work well.

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

@laney, @julian,

Please help to review it.

Revision history for this message
Julian Andres Klode (juliank) wrote :

Should be using apt.ProblemResolver(cache).resolve_by_keep() I think instead of writing your own keep back code.

Arguably the mark_upgrade() calls are wrong; the function does not expose the auto_inst and auto_fix arguments, and hence the resolver now runs at each upgrade and might chose to remove packages at that point. Better to use

            auto = cachedpkg.is_auto_installed
            cachedpkg.mark_install(auto_fix=False, auto_inst=False, from_user=False)
            cachedpkg.mark_auto(auto)

instead of the cachedpkg.mark_upgrade(from_user=False) calls (and auto_inst=True in the second loop). This is basically the same as for new installs, but restores the automatically installed flag to what was there before.

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

@julian,

I have pushed another commit 02999ad.
Please help to review it.

Revision history for this message
Julian Andres Klode (juliank) wrote :

Looks OK. Note that we have apt.ProblemResolver wrapper too which directly takes the apt.Cache, and we have cache.broken_count wrapper for cache._depcache.broken_count, but I can clean that up at some point in the future.

Should do some thorough testing of the changes I suppose.

Oh I also just saw this in apt code:

   // ... but it may remove stuff, we need to clean up afterwards again
   for (pkgCache::PkgIterator I = Cache.PkgBegin(); I.end() == false; ++I)
      if (Cache[I].Delete() == true)
▸ Cache.MarkKeep(I, false, false);

aka auto_inst=True might also mark stuff for removal, so if we really want to make sure we don't remove anything we'd also have to do that:

for pkg in cache:
  if pkg.marked_delete:
    pkg.mark_keep()

just before we call the resolver.

Revision history for this message
Julian Andres Klode (juliank) wrote :

Oh, the second loop got lost that is called with auto_inst=True, it should be added back

Revision history for this message
Julian Andres Klode (juliank) wrote :

Oh, I don't think we want to reset the auto flag if the package is not installed yet

Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

I pushed another commit e725707 that I applied it on the top of origin/focal <20.04.15.3>
And it seems to work fine. https://paste.ubuntu.com/p/Gd9bVNM2Nb/
Previous ibus broken package has been fixed.

Revision history for this message
Julian Andres Klode (juliank) :
review: Approve
Revision history for this message
Shih-Yuan Lee (fourdollars) wrote :

I verified https://paste.ubuntu.com/p/fzK3BQhH7M/ by the fixed broken order and it also works fine.
https://paste.ubuntu.com/p/8B3M6dS8xr/ is the log.

Revision history for this message
Iain Lane (laney) wrote :

Approved by me, jibel is testing & can merge once happy

review: Approve
Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

Tested on hirsute, and no issue found. Thanks.

review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches