Skip to content

Ensure that Gem::Platform parses strings to a fix point#8584

Merged
segiddins merged 4 commits intomasterfrom
segiddins/ensure-that-gem-platform-parses-strings-to-a-fix-point
May 14, 2025
Merged

Ensure that Gem::Platform parses strings to a fix point#8584
segiddins merged 4 commits intomasterfrom
segiddins/ensure-that-gem-platform-parses-strings-to-a-fix-point

Conversation

@segiddins
Copy link
Copy Markdown
Contributor

@segiddins segiddins commented Mar 24, 2025

The issue was that the property that

platform = Gem::Platform.new $string
platform == Gem::Platform.new(platform.to_s)

was not always true.

This property (of acchieving a fix point) is important,
since Gem::Platform gets serialized to a string and
then deserialized back to a Gem::Platform object.
If it doesn't deserialize to the same object, then
different platforms are used for the initial serialization
than subsequent runs.

I used https://github.com/segiddins/Scratch/blob/main/2025/03/rubygems-platform.rb
to find the failing cases and then fixed them.
With this patch, the prop check test now passes.

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

deivid-rodriguez commented Mar 25, 2025

Nice! Indeed it seems important to make that property hold, and nice job finding failing test cases.

At this point, I think the implementation of Gem::Platform#initialize is too complicated so I gave simplifying it a try, while also trying to make this property hold.

How about the following:

diff --git a/lib/rubygems/platform.rb b/lib/rubygems/platform.rb
index 450c2141676..ebb7f3dcdae 100644
--- a/lib/rubygems/platform.rb
+++ b/lib/rubygems/platform.rb
@@ -88,56 +88,44 @@ def initialize(arch)
     when Array then
       @cpu, @os, @version = arch
     when String then
-      arch = arch.split "-"
-
-      if arch.length > 2 && !arch.last.match?(/\d+(\.\d+)?$/) # reassemble x86-linux-{libc}
-        extra = arch.pop
-        arch.last << "-#{extra}"
-      end
-
-      cpu = arch.shift
+      cpu, os = arch.gsub(/-+$/, "").split "-", 2
 
       @cpu = case cpu
              when /i\d86/ then "x86"
              else cpu
       end
 
-      if arch.length == 2 && arch.last.match?(/^\d+(\.\d+)?$/) # for command-line
-        @os, @version = arch
-        return
-      end
-
-      os, = arch
       if os.nil?
         @cpu = nil
         os = cpu
       end # legacy jruby
 
       @os, @version = case os
-                      when /aix(\d+)?/ then             ["aix",       $1]
+                      when /aix-?(\d+)?/ then           ["aix",       $1]
                       when /cygwin/ then                ["cygwin",    nil]
-                      when /darwin(\d+)?/ then          ["darwin",    $1]
+                      when /darwin-?(\d+)?/ then        ["darwin",    $1]
                       when /^macruby$/ then             ["macruby",   nil]
-                      when /freebsd(\d+)?/ then         ["freebsd",   $1]
+                      when /^macruby-?([\d.]+)?/ then   ["macruby",   $1]
+                      when /freebsd-?(\d+)?/ then       ["freebsd",   $1]
                       when /^java$/, /^jruby$/ then     ["java",      nil]
-                      when /^java([\d.]*)/ then         ["java",      $1]
-                      when /^dalvik(\d+)?$/ then        ["dalvik",    $1]
+                      when /^java-?(\d+(?:\.\d+)*)?/ then ["java",    $1]
+                      when /^dalvik-?(\d+)?$/ then      ["dalvik",    $1]
                       when /^dotnet$/ then              ["dotnet",    nil]
-                      when /^dotnet([\d.]*)/ then       ["dotnet",    $1]
+                      when /^dotnet-?(\d+(?:\.\d+)*)?/ then ["dotnet", $1]
                       when /linux-?(\w+)?/ then         ["linux",     $1]
                       when /mingw32/ then               ["mingw32",   nil]
                       when /mingw-?(\w+)?/ then         ["mingw",     $1]
-                      when /(mswin\d+)(\_(\d+))?/ then
+                      when /(mswin\d+)[-_]?(\d+)?/ then
                         os = $1
-                        version = $3
+                        version = $2
                         @cpu = "x86" if @cpu.nil? && os =~ /32$/
                         [os, version]
                       when /netbsdelf/ then             ["netbsdelf", nil]
-                      when /openbsd(\d+\.\d+)?/ then    ["openbsd",   $1]
-                      when /solaris(\d+\.\d+)?/ then    ["solaris",   $1]
+                      when /openbsd-?(\d+\.\d+)?/ then  ["openbsd",   $1]
+                      when /solaris-?(\d+\.\d+)?/ then  ["solaris",   $1]
                       when /wasi/ then                  ["wasi",      nil]
                       # test
-                      when /^(\w+_platform)(\d+)?/ then [$1,          $2]
+                      when /^(\w+_platform)-?(\d+)?/ then [$1, $2]
                       else ["unknown", nil]
       end
     when Gem::Platform then
@@ -154,7 +142,7 @@ def to_a
   end
 
   def to_s
-    to_a.compact.join "-"
+    to_a.compact.join(@cpu.nil? ? "" : "-")
   end
 
   ##

Also, while looking into this I found one case that your initial patch did not catch: x86_64-macruby-x86, and I'd also suggest to add x86_64-dotnetx86, x86_64-dalvik0, and x86_64-dotnet1., because I did not get those right initially and only caught them after running your script to find issues against my initial patch.

@segiddins
Copy link
Copy Markdown
Contributor Author

@deivid-rodriguez done!

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Mmm, I must've messed something up because this seems to break existing specs 🤔

segiddins added 3 commits May 8, 2025 14:33
The issue was that the property that

```ruby
platform = Gem::Platform.new $string
platform == Gem::Platform.new(platform.to_s)
```

was not always true.

This property (of acchieving a fix point) is important,
since `Gem::Platform` gets serialized to a string and
then deserialized back to a `Gem::Platform` object.
If it doesn't deserialize to the same object, then
different platforms are used for the initial serialization
than subsequent runs.

I used https://github.com/segiddins/Scratch/blob/main/2025/03/rubygems-platform.rb
to find the failing cases and then fixed them.
With this patch, the prop check test now passes.
Signed-off-by: Samuel Giddins <segiddins@segiddins.me>
Based on PR feedback

Signed-off-by: Samuel Giddins <segiddins@segiddins.me>
@deivid-rodriguez deivid-rodriguez force-pushed the segiddins/ensure-that-gem-platform-parses-strings-to-a-fix-point branch from 2389dd6 to 40ca3b9 Compare May 8, 2025 12:34
@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Figured it out, a small tweak was missing from the diff I had proposed.

@deivid-rodriguez deivid-rodriguez force-pushed the segiddins/ensure-that-gem-platform-parses-strings-to-a-fix-point branch from 40ca3b9 to 407c1cb Compare May 8, 2025 12:36
@segiddins segiddins merged commit c7b3345 into master May 14, 2025
84 checks passed
@segiddins segiddins deleted the segiddins/ensure-that-gem-platform-parses-strings-to-a-fix-point branch May 14, 2025 19:06
@deivid-rodriguez deivid-rodriguez changed the title Ensure that Gem::Platform parses strings to a fix point Ensure that Gem::Platform parses strings to a fix point Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants