rails-sqlserver / tiny_tds

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

Support Ruby 3.1 and 3.2 #533

Closed andyundso closed 1 year ago

andyundso commented 1 year ago

This PR introduces support for Ruby 3.1 and Ruby 3.2 to tiny_tds. Couple of obstacles:

The GCC compiler for x64 UCRT within the rake-compiler-dock environment is named the same as the one for x64 mingw32, but they yield completely different binaries. This introduces an issue with the port mechanic, where we used the host property from RbConfig to determine the compiler and where to save the ports. I introduced an additional parameter for passing the Ruby architecture to the ports compilation tasks. The Ruby architecture is used to still have a way to differ between the ports for UCRT and Mingw32 while they share the name for the GCC compiler.

The cross-compile job for Windows now runs in parallel for each platform. This was because of another effect: When trying to install the compiled gems for Windows to get the compiled libraries, it raised Unable to find spec for #<Gem::NameTuple tiny_tds, 2.1.5, x64-unknown>, but only for 2.7 and below. Turned out that when installing a gem locally, RubyGems parsed all gem specifications for any file named *.gem in the current directory. And since older versions of Rubygems don't know ucrt, it raised x64-unknown. I found it out by essentially running byebug /my/ruby/installation/bin/gem install tiny_tds, opening the source code of RubyGems to find out where it got the specs from and setting a breakpoint using an absolute path again.

The parallisation added a couple of other issues with CircleCI, namely:

On CircleCI, the Linux tests initially failed for Ruby 3.1 and 3.2 when they tried to compile OpenSSL (see here). I did not bother finding out why, but instead used the ports mechanic from the gem to get the necessary libraries to build tiny_tds.

Note that this PR does not address #515.

ecentell-CPF commented 1 year ago

Any hopes to get this merged soon?

smtadaka commented 1 year ago

Hi, When can we expect this PR to be merged?

ecentell-CPF commented 1 year ago

Hoping to get this PR some attention for a merge

andyundso commented 1 year ago

since a couple of comments have been posted, @aharpervc would you mind to have a look at my PR?

pere73 commented 1 year ago

It would be great, if tiny_tds would support at least Ruby 3.1 on Windows.

ecentell-CPF commented 1 year ago

Bump for some hope to get this merged, thx,

andyundso commented 1 year ago

@aharpervc addressed your feedback, mind to have a second look?

aharpervc commented 1 year ago

@aharpervc addressed your feedback, mind to have a second look?

Seems fine. And the CircleCI integration started working for this PR, which is nice. Something I noticed is that despite having a step to upload artifacts, there isn't any files actually saved. Eg, should be a .gem file here: https://app.circleci.com/pipelines/github/simplificator/tiny_tds/137/workflows/5a2df471-ebdc-4a0c-9fdc-c87af53afb20/jobs/984/artifacts

Having that functional is a goal for this project so that folks can test gem's for PR's without building locally. I suppose it doesn't need to block this PR if there's capacity to investigate soon as a followup

andyundso commented 1 year ago

@aharpervc addressed your feedback, mind to have a second look?

Seems fine. And the CircleCI integration started working for this PR, which is nice. Something I noticed is that despite having a step to upload artifacts, there isn't any files actually saved. Eg, should be a .gem file here: https://app.circleci.com/pipelines/github/simplificator/tiny_tds/137/workflows/5a2df471-ebdc-4a0c-9fdc-c87af53afb20/jobs/984/artifacts

Having that functional is a goal for this project so that folks can test gem's for PR's without building locally. I suppose it doesn't need to block this PR if there's capacity to investigate soon as a followup

Will do so!

ecentell-CPF commented 1 year ago

Thanks for getting this merged! Can you tell me will there be a version increment or a related push to rubygems.org?

aharpervc commented 1 year ago

Yep, if nobody else has made the PR yet, go for it. I recommend this general pattern as a template: https://github.com/rails-sqlserver/tiny_tds/pull/486 (plus a commit that appends .pre to the version string, as a temporary commit to drop before merging).

However...

[re: assets from CI] Will do so!

I recommend basing the version prep PR on 👆, so someone with rubygems access can use the assets from CI for the rubygems release, so we can also validate that CI can produce release-able assets.

ecentell-CPF commented 1 year ago

Prepared PR #540 using #486 as a template.