Code review comment for ~lucaskanashiro/ubuntu/+source/ruby2.7:fix-hash-iteration

Revision history for this message
Bryce Harrington (bryce) wrote :

The patch does look straightforward, and on visual inspection looks good to me.

I didn't spot the package in the PPA though, did it get uploaded? There's an older version, 2.7.0-5ubuntu1.9~ppa2, but the changelog includes another fix, and I'm unsure if the autopkgtest results for that version are valid?

  - ruby2.7/2.7.0-5ubuntu1.9~ppa2
    + ❌ ruby2.7 on focal for ppc64el @ 03.05.23 14:46:37 Log️ 🗒️
      â€¢ run-all FAIL 🟥
      â€¢ bundled-gems PASS 🟩
      â€¢ builtin-extensions PASS 🟩
      â€¢ rubyconfig PASS 🟩

Unfortunately, downgrading to this version did not allow validating the fix:

root@ruby27-hash-iteration:~# apt-cache policy ruby
  Installed: 1:2.7+1
  Candidate: 1:2.7+1
  Version table:
 *** 1:2.7+1 500
        500 focal/main amd64 Packages
        100 /var/lib/dpkg/status
root@ruby27-hash-iteration:~# apt-cache policy ruby2.7
  Installed: 2.7.0-5ubuntu1.9
  Candidate: 2.7.0-5ubuntu1.9
  Version table:
 *** 2.7.0-5ubuntu1.9 500
        500 focal-updates/main amd64 Packages
        500 focal-security/main amd64 Packages
        100 /var/lib/dpkg/status
     2.7.0-5ubuntu1.9~ppa2 500
        500 focal/main amd64 Packages
     2.7.0-5ubuntu1 500
        500 focal/main amd64 Packages
root@ruby27-hash-iteration:~# apt-get install ruby=2.7.0-5ubuntu1.9~ppa2
Reading package lists... Done
Building dependency tree
Reading state information... Done
E: Version '2.7.0-5ubuntu1.9~ppa2' for 'ruby' was not found
root@ruby27-hash-iteration:~# apt-get install ruby2.7=2.7.0-5ubuntu1.9~ppa2
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following package was automatically installed and is no longer required:
Use 'apt autoremove' to remove it.
The following packages will be DOWNGRADED:
0 upgraded, 0 newly installed, 1 downgraded, 0 to remove and 0 not upgraded.
Need to get 108 kB of archives.
After this operation, 0 B of additional disk space will be used.
Do you want to continue? [Y/n]
Get:1 focal/main amd64 ruby2.7 amd64 2.7.0-5ubuntu1.9~ppa2 [108 kB]
Fetched 108 kB in 1s (142 kB/s)
dpkg: warning: downgrading ruby2.7 from 2.7.0-5ubuntu1.9 to 2.7.0-5ubuntu1.9~ppa2
(Reading database ... 33926 files and directories currently installed.)
Preparing to unpack .../ruby2.7_2.7.0-5ubuntu1.9~ppa2_amd64.deb ...
Unpacking ruby2.7 (2.7.0-5ubuntu1.9~ppa2) over (2.7.0-5ubuntu1.9) ...
Setting up ruby2.7 (2.7.0-5ubuntu1.9~ppa2) ...
Processing triggers for man-db (2.9.1-1) ...
root@ruby27-hash-iteration:~# ruby bug.rb
Traceback (most recent call last):
 1: from bug.rb:2:in `<main>'
bug.rb:2:in `each': ret: 2, hash modified during iteration (RuntimeError)

I think it'd be worthwhile to validate this in a PPA since the effect of the RHASH_AR_TABLE_REF() macro is not immediately obvious.

The SRU text looks good, I've run through the test case to verify it reproduces the issue (it does) and once the ppa is at hand can repro the fix as well. I might suggest improving the Impact statement to more directly emphasize how users will be encountering the bug, although the test case makes this pretty evident, so maybe it's ok. All else looks fine.

Anyway, I'd like to do a validation of the fix but otherwise count this as a +1, LGTM.

review: Needs Information

« Back to merge proposal