Syntax highlighting: text
<mruffell> rbasak, bdmurray Robie, would you like me to do some additional testing to cover apt_btrfs_snapshot.py?
<bdmurray> mruffell: no, its just a variable name change from what I can see
<rbasak> bdmurray: it's not covered in https://wiki.ubuntu.com/StableReleaseUpdates#ubuntu-release-upgrader_and_python-apt. Do we need to change it?
<rbasak> It also seems weird to end up with non-deliberate changes like that in SRUs?
<bdmurray> Running pre-build.sh takes a deliberate step
<rbasak> If it was deliberate, you'd have noted it in the commit message and changelog entry.
<bdmurray> What do you want here Robie this is the second time I've uploaded this.
<rbasak> Sorry if I missed it in my previous review. I don't think it should be there.
<rbasak> That seems like a harsh comment to make given that I think we're all agreed that the first upload was objectively buggy/racy. I don't think it's reasonable to push back on legit review issues. I think this issue is also legit, because it's explicitly documented SRU policy that we don't make unrelated changes, and there is no exception documented.
<rbasak> It's perhaps also worth stating that the code can't be verified "obviously correct" from the diff alone. I could check in more detail, but why am I doing that if it shouldn't be there in the first place?
<mruffell> For /usr/lib/python3/dist-packages/apt_btrfs_snapshot.py to change, apt-btrfs-snapshot must have been changed via SRU. Let me try locate its LP bug
<rbasak> Thanks mruffell, but can't we just take out this change?
<mruffell> Hmm, I am wrong, seems apt-btrfs-snapshot has not changed at all, its still the -release version: https://launchpad.net/ubuntu/+source/apt-btrfs-snapshot/3.5.3
<bdmurray> There's a new in the queue
<mruffell> How on earth did this file change then...
<mruffell> bdmurray: did you run pre-build.sh from a Focal system?
<mruffell> Or was this on something newer that has /usr/lib/python3/dist-packages/apt_btrfs_snapshot.py different to Focal release pocket
<bdmurray> I did run pre-build.sh on a newer system IIRC the goal is to have fixes in apt_btrfs_snapshot from the release to which you are upgrading
<mruffell> It seems it was fixed in Hirsute onward: https://bugs.launchpad.net/ubuntu/+source/apt-btrfs-snapshot/+bug/1698977
-ubottu/#ubuntu-devel- Launchpad bug 1698977 in apt-btrfs-snapshot (Ubuntu) "ftbfs in artful" [High, Fix Released]
<mruffell> So you ran it on a Jammy system. ubuntu-release-upgrader is run on a Bionic system to upgrade to Focal, maybe Robie has a point
<mruffell> (but I do agree its a variable rename and shouldn't change any functionality)
<rbasak> I think if the file changes, the behaviour that it might affect should be part of the test plan. Even if it changed as a result of an SRU, the release upgrade wouldn't have been tested. That doesn't seem right to me.
<rbasak> I agree it's just a variable rename, but 1) humans are really bad at spotting typos; and 2) it's not just within the loop, since in Python the variable remains in scope afterwards. So while I agree I can't see a typo, I'd want to see the code exercised in place if it's going to be changed, or some other thought out plan to ensure that a typo doesn't lead to a regression.
<rbasak> Or, IOW, when I review, I don't look for typos, I look for processes to ensure that typos can't slip past.
<mruffell> rbasak: Brian just uploaded a new package to the unapproved queue with the hunk removed.
<rbasak> ack - I just accepted
<rbasak> If we want to accept those in the future then I'm open to that, but I would like to understand what we're doing to ensure such changes are tested.
<mruffell> Maybe we need to add a note that ubuntu-release-upgrader needs to be prepared for build on the series it was intended to be used on, since it does blind copying of python scripts
<bdmurray> rbasak: Your concerns regarding apt_btrfs_snapshot.py are reasonable it just hasn't ever come up before in all my time SRU'ing it.