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

Add Ruby 3.3 to the cross compile list #548

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

ecentell-CPF
Copy link
Contributor

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.

Add Ruby 3.3 to the cross compile list
@andyundso
Copy link
Contributor

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.

Update rake-compiler-dock
@ecentell-CPF
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Attempted fix for 3.0 being found in 3.3.0
@ecentell-CPF
Copy link
Contributor Author

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

Copy link
Contributor

@andyundso andyundso left a comment

Choose a reason for hiding this comment

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

looks good!

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

@aharpervc
Copy link
Contributor

@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
Copy link
Contributor Author

Removed the PRE from version and ChangeLog

@andyundso
Copy link
Contributor

@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
Copy link
Contributor Author

Thanks! Have a great weekend.

@andyundso andyundso merged commit cac41d7 into rails-sqlserver:master Jan 8, 2024
25 checks passed
andyundso added a commit to andyundso/tiny_tds that referenced this pull request Jan 8, 2024
This job tests the version without the precompiled assets on Windows. I missed that rails-sqlserver#548 did not contain it, so this commits adds it in retrospective.
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.

3 participants