Update Windows symlink patch based on PR reviews

- Move non-admin test down to correct section
- Add comments about non-admin user creation
- Use block version of IO.pipe
- Use variables for user name and password
- Move cleanup per File.unlink out of tested+rescued block
- Omit test completely instead of error prone handling in rescue branch
- Use a dedicated method on Windows to create symlinks

Author: larskanis

.github/workflows/rubygems.yml

@@ -4,6 +4,8 @@ on:
   pull_request:
 
   push:
+    branches:
+      - master
 
 concurrency:
   group: ci-${{ github.ref }}-${{ github.workflow }}
@@ -61,11 +63,6 @@ jobs:
           bundler: none
       - name: Install Dependencies
         run: bin/rake setup
-      - name: Run Test with non-Admin user
-        run: |
-          gem inst win32-process --no-doc --conservative
-          ruby bin/windows_run_as_user ruby -S rake test
-        if: matrix.symlink == 'off'
       - name: Run Test
         run: bin/rake test
         if: matrix.ruby.name != 'truffleruby' && matrix.ruby.name != 'jruby' && matrix.symlink != 'off'
@@ -78,6 +75,11 @@ jobs:
       - name: Run Test (Truffleruby)
         run: TRUFFLERUBYOPT="--experimental-options --testing-rubygems" bin/rake test
         if: matrix.ruby.name == 'truffleruby'
+      - name: Run Test with non-Admin user
+        run: |
+          gem inst win32-process --no-doc --conservative
+          ruby bin/windows_run_as_user ruby -S rake test
+        if: matrix.symlink == 'off'
 
     timeout-minutes: 60
 

bin/windows_run_as_user

@@ -1,30 +1,36 @@
 require "win32/process"
 require "rbconfig"
 
-TESTUSER = "testuser"
+testuser = "testuser"
+testpassword = "Password123+"
 
-system("net user #{TESTUSER} /del 2>NUL")
-system("net user #{TESTUSER} \"Password123+\" /add") || raise
-system("icacls . /grant #{TESTUSER}:(OI)(CI)(IO)(F)")
+# Remove a previous test user if present
+system("net user #{testuser} /del 2>NUL")
+# Create a new non-admin user
+system("net user #{testuser} \"#{testpassword}\" /add")
+# Give the new user full access permission on the working directory
+system("icacls . /grant #{testuser}:(OI)(CI)(IO)F")
 
-stdout_read, stdout_write = IO.pipe
-cmd = ARGV.join(" ")
-env = {
+pinfo = nil
+IO.pipe do |stdout_read, stdout_write|
+  cmd = ARGV.join(" ")
+  env = {
     "TMP" => "#{Dir.pwd}/tmp",
     "TEMP" => "#{Dir.pwd}/tmp"
-}
-pinfo = Process.create command_line: cmd,
-    with_logon: TESTUSER,
-    password: "Password123+",
+  }
+  pinfo = Process.create command_line: cmd,
+    with_logon: testuser,
+    password: testpassword,
     cwd: Dir.pwd,
     environment: ENV.to_h.merge(env).map{|k,v| "#{k}=#{v}" },
     startup_info: { stdout: stdout_write, stderr: stdout_write }
 
-stdout_write.close
-out = stdout_read.read
-puts out
+  stdout_write.close
+  out = stdout_read.read
+  puts out
+end
 
 # Wait for process to terminate
-sleep 0.1 while !(ecode=Process.get_exitcode(pinfo.process_id))
+sleep 1 while !(ecode=Process.get_exitcode(pinfo.process_id))
 
 exit ecode

lib/rubygems/package.rb

@@ -470,7 +470,7 @@ def extract_tar_gz(io, destination_dir, pattern = "*") # :nodoc:
 
     symlinks.each do |name, target, destination, real_destination|
       if File.exist?(real_destination)
-        create_symlink_safe(target, destination)
+        create_symlink(target, destination)
       else
         alert_warning "#{@spec.full_name} ships with a dangling symlink named #{name} pointing to missing #{target} file. Ignoring"
       end
@@ -726,14 +726,19 @@ def limit_read(io, name, limit)
     bytes
   end
 
-  # Create a symlink and fallback to copy the file or directory on Windows,
-  # where symlink creation needs special privileges in form of the Developer Mode.
-  def create_symlink_safe(old_name, new_name) # :nodoc:
-    File.symlink(old_name, new_name)
-  rescue Errno::EACCES
-    raise unless Gem.win_platform?
-    from = File.expand_path(old_name, File.dirname(new_name))
-    FileUtils.cp_r(from, new_name)
+  if Gem.win_platform?
+    # Create a symlink and fallback to copy the file or directory on Windows,
+    # where symlink creation needs special privileges in form of the Developer Mode.
+    def create_symlink(old_name, new_name)
+      File.symlink(old_name, new_name)
+    rescue Errno::EACCES
+      from = File.expand_path(old_name, File.dirname(new_name))
+      FileUtils.cp_r(from, new_name)
+    end
+  else
+    def create_symlink(old_name, new_name)
+      File.symlink(old_name, new_name)
+    end
   end
 end
 

test/rubygems/helper.rb

@@ -1268,10 +1268,10 @@ def symlink_supported?
     if @@symlink_supported.nil?
       begin
         File.symlink(File.join(@tempdir, "a"), File.join(@tempdir, "b"))
-        File.unlink(File.join(@tempdir, "b"))
       rescue NotImplementedError, SystemCallError
         @@symlink_supported = false
       else
+        File.unlink(File.join(@tempdir, "b"))
         @@symlink_supported = true
       end
     end

test/rubygems/test_gem_installer.rb

@@ -763,7 +763,7 @@ def test_generate_bin_with_dangling_symlink
       assert_empty errors
     else
       assert_match(/Unable to use symlinks, installing wrapper/i,
-                    errors.to_s)
+                   errors.to_s)
     end
     assert_empty @ui.output
   end

test/rubygems/test_gem_package.rb

@@ -175,6 +175,9 @@ def test_add_files
   end
 
   def test_add_files_symlink
+    unless symlink_supported?
+      omit("symlink - developer mode must be enabled on Windows")
+    end
     spec = Gem::Specification.new
     spec.files = %w[lib/code.rb lib/code_sym.rb lib/code_sym2.rb]
 
@@ -185,16 +188,8 @@ def test_add_files_symlink
     end
 
     # NOTE: 'code.rb' is correct, because it's relative to lib/code_sym.rb
-    begin
-      File.symlink("code.rb", "lib/code_sym.rb")
-      File.symlink("../lib/code.rb", "lib/code_sym2.rb")
-    rescue Errno::EACCES => e
-      if Gem.win_platform?
-        pend "symlink - developer mode must be enabled on Windows"
-      else
-        raise e
-      end
-    end
+    File.symlink("code.rb", "lib/code_sym.rb")
+    File.symlink("../lib/code.rb", "lib/code_sym2.rb")
 
     package = Gem::Package.new "bogus.gem"
     package.spec = spec
@@ -621,7 +616,7 @@ def test_extract_tar_gz_symlink_directory
   end
 
   def test_extract_symlink_into_symlink_dir
-    pend "Symlinks not supported or not enabled" unless symlink_supported?
+    omit "Symlinks not supported or not enabled" unless symlink_supported?
     package = Gem::Package.new @gem
     tgz_io = util_tar_gz do |tar|
       tar.mkdir       "lib", 0o755