rails-sqlserver / tiny_tds

TinyTDS - Simple and fast FreeTDS bindings for Ruby using DB-Library.
Other
605 stars 191 forks source link

Add Ruby 3.3 to the cross compile list #548

Closed ecentell-CPF closed 6 months ago

ecentell-CPF commented 6 months ago

Add Ruby 3.3 to the cross compile list

This is based off work done previously in #530 by @andyundso and #540

Unsure if there are other underlying issues and welcome input.

andyundso commented 6 months ago

think there are two problems here.

  1. The Gem is not precompiled for Ruby 3.3. You need to update rake-compiler-dock to v1.4.0, once in the gemspec file and once in the pipeline at .circleci/config.yml:246.
  2. Our script to install the Ruby version (starting after line 23 in .circleci/config.yml) installs Ruby 3.3 for the job that is supposed to run 3.0. I assume because 3.0 matches 3.3.0. I do not have a solution for this at the moment. Maybe you are proficient in PowerShell and can improve the script to prevent this from happening? Otherwise I look into it later this week.
ecentell-CPF commented 6 months ago

Thanks for your help :)

  1. I have pushed a new commit to update to v1.4.0
  2. When I manually substituted 3.3 for parameters.ruby_version in line 26 and then manually run lines 23-34 locally it does download Ruby 3.3.0-1-x64.exe , clicking the circleCI details for 3.3 shows for step "download and install ruby devkit" Ruby Target Version Found: 3.3.0-1 Download finished, starting installation of 3.3.0-1 Looks like its okay now?
andyundso commented 6 months ago

I was talking about the job that is supposed to run tests against Ruby 3.0. If you look at the logs there, you can see that it also uses Ruby 3.3 instead of 3.0.

ecentell-CPF commented 6 months ago

I see what you mean now. Running code:

        $uri = 'https://api.github.com/repos/oneclick/rubyinstaller2/tags?per_page=200'
        $releases = ((Invoke-WebRequest $uri) | ConvertFrom-Json).name | select-string -Pattern "3.0"
            echo "Releases Found: $releases"
        $target_release = (($releases | Sort-Object -Descending)[0] | Out-String).Trim()
            echo "Target Release Found: $target_release"

Releases Found: RubyInstaller-3.3.0-1 RubyInstaller-3.0.6-1 RubyInstaller-3.0.5-1 RubyInstaller-3.0.4-1 RubyInstaller-3.0.3-1 RubyInstaller-3.0.2-1 RubyInstaller-3.0.1-1 RubyInstaller-3.0.0-1 Target Release Found: RubyInstaller-3.3.0-1

Due to 3.0 being found in RubyInstaller-3.3.0-1

If I expand the search string to include the start of the file name it returns properly:

        $uri = 'https://api.github.com/repos/oneclick/rubyinstaller2/tags?per_page=200'
        $releases = ((Invoke-WebRequest $uri) | ConvertFrom-Json).name | select-string (-join("RubyInstaller-" , "3.0" ))
            echo "Releases Found: $releases"
        $target_release = (($releases | Sort-Object -Descending)[0] | Out-String).Trim()
            echo "Target Release Found: $target_release"

Releases Found: RubyInstaller-3.0.6-1 RubyInstaller-3.0.5-1 RubyInstaller-3.0.4-1 RubyInstaller-3.0.3-1 RubyInstaller-3.0.2-1 RubyInstaller-3.0.1-1 RubyInstaller-3.0.0-1 Target Release Found: RubyInstaller-3.0.6-1

I tested that for all our variables 2.4, 2.5, 2.6, 2.7, 3.0, 3.1, 3.2 and 3.3 So if my PowerShell variables understanding is good enough can we modify line 26 to:

        $releases = ((Invoke-WebRequest $uri) | ConvertFrom-Json).name | select-string (-join("RubyInstaller-" , "<< parameters.ruby_version >>" ))
andyundso commented 6 months ago

My PowerShell skills are rather limited, but your suggestion looks reasonable, so let's try it out.

ecentell-CPF commented 6 months ago

Pushed a new commit. Checked out 3.0 details

Ruby Target Version Found: 3.0.6-1 Download finished, starting installation of 3.0.6-1

Verified the others also had the correct version, so that looks like a valid fix

aharpervc commented 6 months ago

@aharpervc I would propose that we ship a release with just this PR. any objections?

Fine with me 👍, although I recommend dropping the version change from this PR before merging. Also I don't think this needs a prerelease, just go for it and see if anyone complains.

ecentell-CPF commented 6 months ago

Removed the PRE from version and ChangeLog

andyundso commented 6 months ago

@ecentell-CPF I want to do couple of manual tests before merging the PR. I likely have time at the start of next week to do this, then I would release the new version when everything goes smooth.

ecentell-CPF commented 6 months ago

Thanks! Have a great weekend.