Skip to content
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

Ruby 3.2 yast2-storage-ng sometimes fails to build, diff in Marshal.dump #1321

Merged
merged 7 commits into from
Mar 3, 2023

Conversation

mvidner
Copy link
Member

@mvidner mvidner commented Feb 21, 2023

Problem

Ruby 3.2 yast2-storage-ng sometimes fails to build, diff in Marshal.dump

The failing test means to check that a new ProposalSettings is in fact equal to the original one, and does so by dumping and reloading the two objects with Marshal. It fails in case the two objects differ in the order in which their instance variables were assigned.

Solution

Instead of using Marshal, add ProposalSettings#==.

Do it by introducing a simple module, EqualByInstanceVariables, including it in that class and a couple of others that are used by it.

Gory details: everything I've learned about equality in Ruby along the way, very messy so far: https://gist.github.com/mvidner/eb10bf4d70f0df5c1073a2229fd4491e

Testing

  • Added new unit tests

Screenshots

N/A

@mvidner
Copy link
Member Author

mvidner commented Feb 21, 2023

Today's findings:

The Marshal dumps are different because some otherwise equal objects objects have different ordering of instance variables, namely in VolumeSpecification objects. Why, yes, I did spend a stupid amount of time to find the reason for this difference. Unsuccessfully.

My plan is to accept the reality of the nondeterministic ordering and instead remove the gorilla from the room: Marshal. We are using it because ProposalSettings#== does not work properly

WIP:

irb(main):001:0> class C; def initialize(attrs); attrs.each { |a| instance_variable_set(a.to_sym, a.to_s) } ; end ; end
=> :initialize
irb(main):004:0> a = C.new([])
=> #<C:0x0000563aa62d27b0>
irb(main):005:0> b = C.new([])
=> #<C:0x0000563aa618b848>
irb(main):006:0> a == b
=> false
irb(main):007:0> module EqualByIVs; def ==(other); return if other.class != self.class; instance_variables.all? { |iv| instance_variable_get(iv) == other.instance_variable_get(iv) }; end; end
=> :==
irb(main):008:0> class C; include EqualByIVs; end
=> C
irb(main):009:0> a == b
=> true

A word-diff is useful:
git diff --color-words /var/tmp/build-root-standard/tmp/dump[12]
… values"

This reverts commit 21b4a13.

The debug print revealed that there is sometimes a different ordering of
instance variables in VolumeSpecification than the one used by
initialize/apply_defaults

Don't know why, just live with it now
@mvidner mvidner force-pushed the ruby32-proposal-settings-comparison branch from ad50a9b to 984e54b Compare February 24, 2023 09:34
@coveralls
Copy link

coveralls commented Feb 24, 2023

Coverage Status

Coverage: 97.755% (+0.001%) from 97.754% when pulling 567058c on ruby32-proposal-settings-comparison into 7b60019 on master.

@mvidner mvidner force-pushed the ruby32-proposal-settings-comparison branch from 984e54b to 90387ef Compare February 27, 2023 08:52
@mvidner mvidner force-pushed the ruby32-proposal-settings-comparison branch from 90387ef to 2ebbab7 Compare March 2, 2023 10:17
@mvidner mvidner marked this pull request as ready for review March 2, 2023 10:18
@mvidner mvidner force-pushed the ruby32-proposal-settings-comparison branch from 2ebbab7 to 567058c Compare March 2, 2023 10:24
# specify the instance variables used for the comparison:
# https://github.com/yast/yast-yast2/blob/master/library/general/src/lib/yast2/equatable.rb
module EqualByInstanceVariables
# TODO: move to base library?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mvidner mvidner merged commit e316f1f into master Mar 3, 2023
@mvidner mvidner deleted the ruby32-proposal-settings-comparison branch March 3, 2023 08:19
@yast-bot
Copy link

yast-bot commented Mar 3, 2023

✔️ Public Jenkins job #452 successfully finished
✔️ Created OBS submit request #1069105

@yast-bot
Copy link

yast-bot commented Mar 3, 2023

✔️ Internal Jenkins job #1138 successfully finished
✔️ Created IBS submit request #291247

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants