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

Windows - Add support for ucrt & Windows-2022 image #224

Merged
merged 15 commits into from
Jan 13, 2022

Conversation

MSP-Greg
Copy link
Collaborator

@MSP-Greg MSP-Greg commented Oct 8, 2021

See commits. On Windows-2022 image, code adds the MSYS2 packages from 'base-devel' group needed for Ruby builds.

Also adds ENV variables by running ridk enable, testing for the file's existence first. Uses code similar to the code for mswin's vcvars script. Parses the output of the set command, so it's not dependent on ridk's output.

Windows 2019 Image - No changes when using with a mingw Ruby. When using a ucrt Ruby, a pre-built ucrt 7z file is installed from https://github.com/MSP-Greg/setup-msys2-gcc/releases/tag/msys2-gcc-pkgs. The 7z files are generated three times a day in that repo.

Windows 2022 Image - Since the image has no MSYS2 build tools installed, both an MSYS2 package and a gcc package (mingw or ucrt) are installed from the MSP-Greg/setup-msys2-gcc repo.

@MSP-Greg MSP-Greg force-pushed the win-ucrt-1 branch 2 times, most recently from 167d4f5 to d1c58dc Compare November 15, 2021 00:35
@MSP-Greg
Copy link
Collaborator Author

I created a fork of ruby-loco, and built both mingw & ucrt on WIndows 2022 using this PR branch for setup-ruby. Both compiled and passed all tests. This PR runs jobs for mingw & ucrt builds on both Windows 2019 & 2022.

@MSP-Greg
Copy link
Collaborator Author

@eregon

Good day.

All the below is Windows specific...

Sorry for all the CI, a few typos, etc. Since windows-2022 does not have MSYS2 build tools installed (there's pro's and con's for that decision), we need to install the tools here, as any CI that installs an extension gem will fail otherwise. With the previous images, we just needed to install the tools for the old Rubies (2.3 and earlier), now that's required for all Rubies.

This is working in ruby/ruby, and I've tested it with nokogiri and puma. nokogiri needs the xml related packages installed, so it's using a branch in setup-ruby-pkgs that pairs with this PR's branch.

I'll check if it works with ruby/openssl, which has an mswin build, and if that's ok, it's ready to go...

bundler.js Outdated Show resolved Hide resolved

// ssl files cause issues with non RI2 Rubies (<2.4) and ruby/ruby's CI from
// build folder due to dll resolution
function renameSystem32Dlls() {
Copy link
Member

Choose a reason for hiding this comment

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

This seems quite risky, is there any other way to solve the issue?
Is it only for Ruby <= 2.3? It seems done for all cases here.
It seems to work fine before for <= 2.3, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is quite involved as to why it's needed.

To be clear, these are not MSFT files. One of the oldest and most popular Windows OpenSSL implementations (built with Visual C) installs the OpenSSL dll's in C:\Windows\System32. Normally, that folder is considered 'off-limits', and should be used only by MSFT.

I've had this in setup-ruby-pkgs for a long time, and it's never caused an issue. It causes all sorts of WTF issues if it's not done...

Copy link
Member

Choose a reason for hiding this comment

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

My concern here is someone might waste a lot of time if for some reason some part of their workflow would use those files and using setup-ruby has the very unexpected side effect to delete or rename files under C:\Windows\System32 (which are not provided by Microsoft but still).
I remember some people were already confused due to the more extensive Path changes on Windows by this action, but this goes another level. If we keep it we should have a clear logged message mentioning what it and why it does it.

I think it's better to not do this. If those files are problematic then filing an issue at https://github.com/actions/virtual-environments seems the best way forward.

@larskanis What do you think? Having a 3rd opinion might be helpful here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Below is info from a repo I've got that spits out a bunch of data on Actions Images, taken from the windows-2022 image job. All of the openssl.exe files have the dll's in their own directory. The C:\Windows\System32 files are added by the same installer that creates the files at C:\Program Files\OpenSSL.

——————————————————————————————————————————————————————————————————————— libcrypto-1_1-x64.dll
C:\mysql\bin\libcrypto-1_1-x64.dll
C:\Windows\System32\libcrypto-1_1-x64.dll
C:\Program Files\OpenSSL\bin\libcrypto-1_1-x64.dll
C:\Program Files\Git\mingw64\bin\libcrypto-1_1-x64.dll
c:\tools\php\libcrypto-1_1-x64.dll

——————————————————————————————————————————————————————————————————————— libssl-1_1-x64.dll
C:\mysql\bin\libssl-1_1-x64.dll
C:\Windows\System32\libssl-1_1-x64.dll
C:\Program Files\OpenSSL\bin\libssl-1_1-x64.dll
C:\Program Files\Git\mingw64\bin\libssl-1_1-x64.dll
c:\tools\php\libssl-1_1-x64.dll

——————————————————————————————————————————————————————————————————————— openssl info
OpenSSL 1.1.1  11 Sep 2018    C:\Program Files\OpenSSL\bin\openssl.exe
OpenSSL 1.1.1l  24 Aug 2021   C:\Program Files\Git\mingw64\bin\openssl.exe
OpenSSL 1.1.1l  24 Aug 2021   C:\Program Files\Git\usr\bin\openssl.exe

My concern here is someone might waste a lot of time

I've worked with quite a few extension gems re Windows, Ruby master/head, and Actions. Never come across a problem. It is a confusing problem if Rubies that use ENV['Path'] for their dll resolution and load them because they're in C:\Windows\System32...

Copy link
Member

@eregon eregon Jan 13, 2022

Choose a reason for hiding this comment

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

Could you do this only for RI1, so Ruby <= 2.3, and log a message in that case?
I think it's more reasonable to use workarounds like this for old versions, but for recent versions we should avoid such changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Windows OpenSSL dll's have different names, depending on the version, the break happens between 1.0.2 and 1.1.0. Updated for Ruby 2.4 and earlier, as some extconf files that work with OpenSSL look for the 1.1.0 names first. Windows Ruby 2.4 uses 1.0.2. See comment

.github/workflows/test.yml Outdated Show resolved Hide resolved
windows.js Outdated Show resolved Hide resolved
windows.js Outdated Show resolved Hide resolved
@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 3, 2022

Re the windows-2022 JRuby failures:

  C:\Windows\system32\tar.exe -xz -C /d/ -f /d/a/_temp/a1828e52-1472-430c-9f5e-f1202ac6f2b7
  tar.exe: Error opening archive: Failed to open '/d/a/_temp/a1828e52-1472-430c-9f5e-f1202ac6f2b7'

Previous images had the MSYS2 tar installed, windows-2022 does not, so the MSFT tar.exe is used, but it needs a Windows path. Looking at it now...

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 3, 2022

@eregon re the Windows JRuby extraction issue, I've got that fixed. I just noticed the 'Windows JRuby' step in CI (gem install sassc). It fails on windows-2022 because there are no build tools.

So, should they be installed? If so, I need to see whether they should be mingw or ucrt?

@eregon
Copy link
Member

eregon commented Jan 3, 2022

So, should they be installed? If so, I need to see whether they should be mingw or ucrt?

I guess it doesn't really matter, so the easier the better.
For sassc, a C++ compiler is needed to create a .dll and that's then loaded via FFI.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 3, 2022

Locally, I built the sassc gem with jruby-head, ruby-mingw, & ruby-ucrt.

Using objdump to see what dll's they load, the following is the data. JRuby may be compiled with MSFT Visual Studio. Not sure about enabling that, but if one were to choose between MinGW & UCRT, the MinGW build's dll's line up with JRuby much better. I'll work on it.

———————————————————————————————————————————————— ruby-mingw
ADVAPI32.dll
bcrypt.dll
imagehlp.dll
IPHLPAPI.DLL
KERNEL32.dll
libgmp-10.dll
msvcrt.dll
SHELL32.dll
USER32.dll
DLL Name: WS2_32.dll

———————————————————————————————————————————————— ruby-ucrt
ADVAPI32.dll
api-ms-win-crt-convert-l1-1-0.dll
api-ms-win-crt-environment-l1-1-0.dll
api-ms-win-crt-filesystem-l1-1-0.dll
api-ms-win-crt-heap-l1-1-0.dll
api-ms-win-crt-locale-l1-1-0.dll
api-ms-win-crt-math-l1-1-0.dll
api-ms-win-crt-private-l1-1-0.dll
api-ms-win-crt-runtime-l1-1-0.dll
api-ms-win-crt-stdio-l1-1-0.dll
api-ms-win-crt-string-l1-1-0.dll
api-ms-win-crt-time-l1-1-0.dll
api-ms-win-crt-utility-l1-1-0.dll
bcrypt.dll
imagehlp.dll
IPHLPAPI.DLL
KERNEL32.dll
libgmp-10.dll
SHELL32.dll
USER32.dll
WS2_32.dll


———————————————————————————————————————————————— MinGW libsass.so
ADVAPI32.dll
KERNEL32.dll
libgcc_s_seh-1.dll
libstdc++-6.dll
msvcrt.dll

———————————————————————————————————————————————— UCRT libsass.so
ADVAPI32.dll
api-ms-win-crt-convert-l1-1-0.dll
api-ms-win-crt-environment-l1-1-0.dll
api-ms-win-crt-filesystem-l1-1-0.dll
api-ms-win-crt-heap-l1-1-0.dll
api-ms-win-crt-locale-l1-1-0.dll
api-ms-win-crt-math-l1-1-0.dll
api-ms-win-crt-private-l1-1-0.dll
api-ms-win-crt-runtime-l1-1-0.dll
api-ms-win-crt-stdio-l1-1-0.dll
api-ms-win-crt-string-l1-1-0.dll
api-ms-win-crt-time-l1-1-0.dll
KERNEL32.dll
libgcc_s_seh-1.dll
libstdc++-6.dll


———————————————————————————————————————————————— Windows JRuby
ADVAPI32.DLL
KERNEL32.dll
msvcrt.dll
USER32.dll
WS2_32.dll

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 3, 2022

Fixed the issues with JRuby & windows-2022. Appears there are problems with Actions macOS servers. Anyway, I think this is complete, with the issue of RubyGems possibly remaining.

BTW, Happy New Year!

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 4, 2022

@eregon & @larskanis

This morning I decided to look at the Bundler/RubyGems issue, as this PR currently updates RubyGems instead of updating Bundler for ((bundlerVersionConstraint === '~> 2') && (common.floatVersion(rubyVersion) >= 2.3)).

So, I ran it my fork of this repo, results. The only failing jobs are Windows 3.1.0.

There are a few repos running from the PR branch of my fork, Puma being one. It has been passing using it.

I reverted the RubyGems installation, results. As above, the Windows 3.1.0 jobs failed, but also Ruby 2.5 jobs on every OS failed. The 2.5 failures maybe due to Psych, don't have time to look this morning.

Another 'issue' in both CI runs is that all jobs using Ruby 2.3 thru 2.5 have a warning shown, similar to the below. In the CI in my fork of of this repo, see 'bundle install' of the 'Run /./' step, in the Puma fork, see 'bundle install' of the 'load ruby' step:

Your RubyGems version (2.6.14.4)) has a bug that prevents `required_ruby_version` from working for Bundler

So, not sure what to do about this. Lars, if you have time, maybe see if the issue with the 3.1 bin can be fixed? I'll try to look later... Regardless, we've got warnings and failures in all OS's for Ruby 2.3 thru 2.5 when upgrading to Bundler 2.3.4...

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 4, 2022

I updated RubyGems/Bundler handling. For Ruby 2.3 thru 2.5 (all EOL), it does gem update --system. For Windows 3.1.0, it installs Bundler with --force. So, it works, no warnings...

@eregon
Copy link
Member

eregon commented Jan 13, 2022

@MSP-Greg Only 2 remaining then I think it should be ready to be merged:
#224 (comment)
#224 (comment)

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you for this great work!

@eregon eregon merged commit fb94cfb into ruby:master Jan 13, 2022
@eregon
Copy link
Member

eregon commented Jan 13, 2022

@MSP-Greg
Copy link
Collaborator Author

I suspect there are things about TruffleRuby that even without a few nights sleep you would be aware of. Given how long I've used Ruby with Windows, same thing. Hence, sorry about not commenting several things.

One change that's contained in MSP-Greg/setup-msys2-gcc is that I included all the packages needed to build Ruby, which is also true for all the extension stdlib/gems in Ruby repos. I was tired of people using MSP-Greg/setup-ruby-pkgs to install packages that are required by Ruby. Also, I thought matching Ubuntu was a good idea.

But, when removing MSP-Greg/setup-ruby-pkgs from CI here and there, there was a fair bit of code that needed to move to this repo. And that's where I got a little light with the comments... Thanks for reviewing this and keeping the repo up to date. Too many people think it's automatic.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 13, 2022

EDIT: I deleted the workflow shown below. Puma uses setup-ruby-pkgs, and there was an odd thing where I pushed a release to it, then immediately used the release in other repos, and the release wasn't updated. Maybe releases take some time to move around GH datacenters, not sure.

Anyway, the weird issue related to Psych (it's in Puma's Gemfile.lock) no longer exists, but the warnings still appear. So, everything is sort of ok. Hence, ignore the below...

———————————————— Ignore below ————————————————

@eregon I just updated Puma's CI in my fork. Due to Psych issues, On all OS's, Ruby 2.5 is failing, and Ruby 2.3 & 2.4 logs warning that I mentioned before in the tests. See https://github.com/MSP-Greg/puma/runs/4808743184.

This was that issue involving:

if ((bundlerVersionConstraint === '~> 2') && (floatVersion >= 2.3) && (floatVersion <= 2.5)) {
  // stop warnings similar to
  // Your RubyGems version (2.6.14.4)) has a bug that prevents `required_ruby_version` from working for Bundler
  await exec.exec(gem, ['update', '--system', '--no-document'])

Not sure what should be done...

Part of the issue is the warnings, and also that new Bundler/RubyGems is compatible with new and old Psych, but old RubyGems is not, so if your bundle/Gemfile update Psych, things explode.

@lopopolo
Copy link

Hi, is it likely that the MSYS changes mentioned here are interfering with libraries available in the base installed packages on the GHA runner?

It looks like these changes combined with using Ruby 3.1.0 on windows-2019 add a non-default libclang to the path which breaks Rust bindgen.

Example build: https://github.com/artichoke/artichoke/runs/4811419721?check_suite_focus=true

@MSP-Greg
Copy link
Collaborator Author

I haven't looked at the build, but Ruby 3.1 is built with the MSYS2 UCRT64 toolset, and Ruby 3.0 (and previous) were built with the MSYS2 MINGW64 toolset.

Not quite sure what you mean by 'non-default libclang'? Nothing has changed in the handling of Path.

@lopopolo
Copy link

bindgen does a dynamic load of libclang. this is the runtime error:

  thread 'main' panicked at 'Unable to find libclang: "the `libclang` shared library at C:\\msys64\\mingw64\\bin\\libclang.dll could not be opened: LoadLibraryExW failed"', C:\Users\Default\.cargo\registry\src\github.com-1ecc6299db9ec823\bindgen-0.59.2\src/lib.rs:2144:31

@MSP-Greg
Copy link
Collaborator Author

When using Ruby 3.1 (or head or ucrt), you'll need to (at a minimum) add the following:

pacman -S mingw-w64-ucrt-x86_64-clang

The windows-latest (windows-2019) image only contains the mingw64 toolset (maybe also mingw32, but we don't use). As mentioned above, Windows Ruby 3.1 uses the ucrt64 toolset, which is not pre-installed on any Actions Windows images.

Note that in the windows-2022 image the mingw64 toolset is not installed, nor are the MSYS2 build tools.

@MSP-Greg
Copy link
Collaborator Author

@MSP-Greg
Copy link
Collaborator Author

@eregon
Time for a new release? ruby/ruby mingw CI is failing on a few openssl tests due to the dll resolution issue. I'm double checking right now.

@eregon
Copy link
Member

eregon commented Jan 14, 2022

@MSP-Greg There is already https://github.com/ruby/setup-ruby/releases/tag/v1.92.0 and the two commits after are just cleanups, shouldn't matter. What's the issue?

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jan 14, 2022

ruby/ruby mingw CI

  Retrying...
  
    1) Failure:
  OpenSSL::TestConfig#test_s_parse_include [D:/a/ruby/ruby/src/test/openssl/test_config.rb:158]:
  <["default", "sec-a", "sec-b", "sec-main"]> expected but was
  <["default", "sec-main"]>.
  
    2) Error:
  OpenSSL::TestCipher#test_aes_ccm:
  OpenSSL::Cipher::CipherError: 
      D:/a/ruby/ruby/src/test/openssl/test_cipher.rb:374:in `ccm_data_len='
      D:/a/ruby/ruby/src/test/openssl/test_cipher.rb:374:in `block (2 levels) in new_decryptor'
      D:/a/ruby/ruby/src/test/openssl/test_cipher.rb:374:in `each'
      D:/a/ruby/ruby/src/test/openssl/test_cipher.rb:374:in `block in new_decryptor'
      <internal:kernel>:90:in `tap'
      D:/a/ruby/ruby/src/test/openssl/test_cipher.rb:372:in `new_decryptor'
      D:/a/ruby/ruby/src/test/openssl/test_cipher.rb:187:in `test_aes_ccm'

I'm testing right now, I believe the SYSTEM32 dll's need to be renamed for all Rubies.

@MSP-Greg
Copy link
Collaborator Author

See #258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants