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 Windows MSYS2 ucrt support #194

Closed
wants to merge 1 commit into from
Closed

Conversation

MSP-Greg
Copy link
Collaborator

@MSP-Greg MSP-Greg commented Jun 17, 2021

Add support for Windows Rubies compiled with the MSYS2 ucrt packages.

On Windows, we currently set ENV['Path'] to include the MSYS2 build tools locations.

This provides basic *nix commands, which are often used in Actions and also build/compile scripts. It also places the gcc compiler in the path.

With non CRuby builds, the path additions need to happen before the Ruby is installed to allow access to the tar command. Currently, all CRuby builds are 7z files, so tar isn't needed.

Now, with ucrt builds available, we need to check the name of the main Ruby dll to determine whether it is mingw or ucrt. Once that is determined, we can set the path additions.

I've opened this PR for review, but it should not be merged MSYS2 ucrt packages are available in the Actions images.

But, I've patched my fork of setup-ruby, and added a branch in setup-ruby-pkgs for it, and it's being used in PR #2272 in Nokogiri, and it's working fine. Note that it must be used with setup-ruby-pkgs, as it is installing all the (missing) ucrt packages...

See actions/runner-images#3597

Closes #193

@MSP-Greg MSP-Greg changed the title @MSP-Greg Add Windows MSYS2 ucrt support, update package.json dependencies Add Windows MSYS2 ucrt support, update package.json dependencies Jun 17, 2021
@MSP-Greg MSP-Greg added dependencies Pull requests that update a dependency file enhancement New feature or request labels Jun 17, 2021
common.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
ruby-builder.js Outdated Show resolved Hide resolved
@MSP-Greg
Copy link
Collaborator Author

@eregon

I've got this finished, but as long as I was working on it, I added/changed two things, maybe you can have a look. The run is at:

https://github.com/MSP-Greg/ruby-setup-ruby/actions/runs/950768152

  1. I added timing (for now) to the Windows 'dir' call, it takes between 7 and 25 mS. Personally, I don't think we need to worry about the time needed.

  2. The 'PATH' step always bothered me, as showing a bash shell PATH for Windows is not ideal. So, I replaced echo $PATH with a Powershell command, which shows each entry on a separate line, and in a manner a Windows user would expect. When running a Powershell step, Actions adds the path to Powershell to PATH, that's the reason for the code, which removes it...

  3. The log for the 'Run /./' step. Rather than only showing the entries removed from PATH, I changed it to show both removed and added entries. Since it's in a 'collapsed' section anyway, seemed like adding it wouldn't affect most people, and the info is helpful. The timing added for 1 will be removed...

Thoughts? Should I remove 2 and 3 from this, and create one or two additional PR's?

@eregon
Copy link
Member

eregon commented Jun 27, 2021

@MSP-Greg Could you rebase this and address the usage of the subshell? Then it should be good to go.

@MSP-Greg MSP-Greg changed the title Add Windows MSYS2 ucrt support, update package.json dependencies Add Windows MSYS2 ucrt support Jun 27, 2021
@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jun 27, 2021

I used 'fs' calls to determine whether the Windows Ruby dll is ucrt or not...

I don't think this should be merged yet, as it's using the wrong compiler for ucrt/ruby-head, and the ucrt packages aren't installed...

common.js Outdated Show resolved Hide resolved
common.js Show resolved Hide resolved
@eregon
Copy link
Member

eregon commented Jun 28, 2021

I don't think this should be merged yet, as it's using the wrong compiler for ucrt/ruby-head, and the ucrt packages aren't installed...

OK, ping me when you think we should merge this then.

Is it a problem for ruby-head on Windows in the meanwhile? Is there any workaround we could use?

@MSP-Greg
Copy link
Collaborator Author

Is it a problem for ruby-head on Windows in the meanwhile?

Yes

Is there any workaround we could use?

IDK. It's complicated. MinGW builds use a version of the MSVC runtime that is 'legacy', but still 'maintained'. UCRT builds essentially use the current MSVC runtime. So one could use a Ruby built with one and an extension gem built with the other, and CI might pass. But, using two may result in intermittent memory problems.

The only solution would be to install the ucrt gcc tools here. At present, that can be done in setup-ruby-packages.

JFYI, the ucrt packages may not be installed on Actions Images.

MSVC = Microsoft Visual C

@MSP-Greg
Copy link
Collaborator Author

In a branch of my fork I added installing ucrt gcc tools when needed. The additional time varied, one job to 35 s, another took 71 s.

Doing that should allow all Actions jobs with Windows ruby-head to compile correctly.

As you know, many repos may not need to compile anything, but if they are extension gems or have them in their Gemfile, they will...

@eregon
Copy link
Member

eregon commented Jun 29, 2021

What about the ruby-loco mingw variant? Maybe we should temporarily replace ruby-head with that so it'd still work for native extensions, and not require quite some extra setup time?

Do you know what it looks when current ruby-head fails due to using the wrong runtime?

https://github.com/ruby/setup-ruby/runs/2925023247?check_suite_focus=true seems to work, but probably because it does not actually load the json extension.

@larskanis
Copy link
Contributor

Maybe we should temporarily replace ruby-head with that so it'd still work for native extensions, and not require quite some extra setup time?

I don't think that's a good option. The request to add any MSYS2-UCRT packages was just declined: actions/runner-images#3597 So we have to add some kind of package installation to ruby-setup for ruby-head sooner or later.

Secondly I didn't switch ruby-head to UCRT to get kicked out of ruby-setup, but to bring the Ruby ecosystem on Windows to a more modern platform and to trigger related tools like ruby-setup.

I like the attempt of @MSP-Greg here. Maybe we should shrink the number of packages to a minimum though, which is gcc and pkgconf IMHO.

Maybe we can add some package cache to speed up the setup, like setup-setup does.

Do you know what it looks when current ruby-head fails due to using the wrong runtime?

The gcc of MSYS2-MINGW64 is picked with the current ruby/setup-ruby@v1 setup. Many small gems work with it successfully. I found that ruby-pg passes all specs with it. But nokogiri fails with a memory error. And ffi fails in some spec that do 80-bit float operations, which is a not very common.

If we merge this PR in the current form, than the the path to C:/MSYS64/mingw64 is no longer used, but switched to an empty path of C:/MSYS64/ucrt64. In this case an older gcc-8 of the Chocolatey MINGW64-W64 target is getting used instead, since it's the next gcc executable in the default PATH. And this gcc builds the ruby-pg extension, but it fails at runtime like so.

@eregon
Copy link
Member

eregon commented Jun 29, 2021

OK, I guess we have no choice but to do runtime installation of some MSYS2-UCRT packages then.

That's unfortunate, as the increased time might cause some gems to stop testing windows + ruby-head in CI (and potentially discourage from testing ruby 3.1 on windows when it comes out). Hopefully the added time is not significant enough compared to total build time to cause that.

@MSP-Greg Could you bring installUCRT() in this PR then, and limit it to gcc and pkgconf?

If users need extra packages, maybe we could link to https://github.com/msys2/setup-msys2 and show an example workflow for it?
Or improve setup-ruby-pkgs so it can do caching too, if that makes sense?
I'd like to avoid extra caching logic just for ucrt in this action if possible.

In this case an older gcc-8 of the Chocolatey MINGW64-W64 target is getting used instead

Should rubyinstaller builds error or warn in such a case, when the found gcc is not from the correct MSYS2 path?

That might make the option to not install anything by default a possibility, if we can have a clear error message.
I'm not sure how frequent it is to have Ruby CIs on Windows without a single native extension dependency, and whether it's worth optimizing for that case. I'd say probably not.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jun 29, 2021

Sorry, AFK.

The last 'ucrt install' iteration installed the dlfcn, make, pkgconf, libyaml, & gcc packages.

I'm not sure why I added libyaml, but I think the packages should mimic what's pre-installed on 'normal' Ubuntu and/or macOS installs. Or, don't make people use 'setup-ruby-pkgs' for packages that are already installed on Ubuntu and/or macOS.

One option, rather than caching ucrt in every repo that is using it, would be to create some type of ucrt package with a cron job either here or in another repo.

Back before Actions added MYS2 to the images, I did something similar. But, doing so with a pre-existing MSYS2 install is a bit different.

A quick look seems to show that updating the MSYS2 package database and downloading the packages takes about the same amount of time as installing the packages. So I'll work on seeing what can be done to speed that up with pre-assembled package.

Lars:

First, we're on the same page, sort of. ucrt needs to be supported. I was waiting to see what ruby/ruby did, but my hope was that ucrt would be considered the 'supported' platform and mingw support would continue for a few versions. I intend to also support ucrt in ruby-loco, probably calling it ruby-ucrt.

Secondly, thanks for your work with ucrt, as a 'push' is always helpful. Conversely (and I'll just say it once), waiting until the infrastructure is in place to support it in CI would have been nice...

@larskanis
Copy link
Contributor

Ideally the github CI should have the same tools preinstalled as the RubyInstaller-Devkit version. But this would slow down the startup too much and it's not too bad to have to specify the exact dependencies of the project, instead of relying on some predefined packages.

Should rubyinstaller builds error or warn in such a case, when the found gcc is not from the correct MSYS2 path?

That's a great idea! The rubyinstaller runtime can check the gcc path when MSYS2 tools are enabled and if it's not within the MSYS2 /ucrt64/bin or /mingw64/bin path, it could print a warning like

**********************************************************************************
****                   Incompatibe compiler detected !                        ****
****                                                                          ****
****  The gcc executabe in the PATH doesn't fit to the running ruby version:  ****
****    gcc version 8.1.0 (x86_64-posix-seh-rev0, Built by MinGW-W64 project) ****
****  C extensions might not compile or crash at runtime.                     ****
****  Please make sure that a suitable compiler is installed be running       ****
****                                                                          ****
****    ridk exec pacman -S mingw-w64-ucrt-x86_64-gcc                         ****
****                                                                          ****
**********************************************************************************

Depending on ENV["CI"] it's also possible to add another suggestion how the CI user should react. I'll try to implement this idea.

@MSP-Greg
Copy link
Collaborator Author

Ideally the github CI should have the same tools preinstalled as the RubyInstaller-Devkit version

Disagree.

I think the closer we can come to installing packages that are likely to be used with Ruby and also exist in the current mingw install is best? For instance, libffi is currently installed, so some existing CI may not be updating it.

See https://github.com/MSP-Greg/actions-image-testing/runs/2922574190?check_suite_focus=true#step:20:9

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jul 2, 2021

I've finished most of the work on a 'faster' means of installing ucrt. An additional repo runs CI (a cron job); it adds the ucrt packages to the image's MSYS2 install, then creates a 7z file with all the additions from ucrt, and saves it in a release. Then, setup-ruby downloads the 7z and extracts it. Total install time for ruby-head & ucrt was about 20 seconds.

Since there was mention of total or partial removal of MSYS2 in the next Windows image (windows-2022?), it can also build mingw64 and mingw32 images. With some minor changes, it could also create an MSYS2 package.

The repo that creates the 7z file is https://github.com/MSP-Greg/setup-msys2-gcc. At present, it's installing the following packages. This can easily be changed:

%w[dlfcn make pkgconf libyaml libmangle-git gcc]

A repo I use for 'image information' has a CI run at:
https://github.com/MSP-Greg/actions-image-testing/runs/2968072844?

Check the windows jobs, and note the output for the 'Install Ruby', 'MSYS2 exe files', and 'MSYS2 packages'.

My fork of setup-ruby has code for this added, see CI at:
https://github.com/MSP-Greg/ruby-setup-ruby/actions/runs/991711737

Thoughts?

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jul 2, 2021

@larskanis

If we add the 'tools-git' package to the above, we've got all the mingw packages you've got listed in 03_dev_tools.rb. I'll add it to the MSP-Greg/setup-msys2-gcc repo.

One should be able to swap ruby/setup-ruby@v1 to MSP-Greg/ruby-setup-ruby@win-ucrt-1 and ruby-head should work just like the other builds. Of course, just for testing...

@larskanis
Copy link
Contributor

It looks like MSP-Greg/ruby-setup-ruby@win-ucrt-1 is more than twice as fast as #197 and #198. It's a bit more complex, but I think it's worth the effort.

Nevertheless the environment variables should be set by RubyInstaller as described in #197.

@MSP-Greg You should not do pacman -Sy here. This updates the package database without updating the MSYS2 system and can lead to an inconsistent state. Also /var/lib/pacman should not be in the 7z file. The package database already includes all UCRT stuff we need. Is there a good reason to update pacman-mirrors in the same line?

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Jul 4, 2021

@eregon & @larskanis

the environment variables should be set by RubyInstaller

Not sure. Have you checked to see whether the environment variables are needed for general build use, or are they needed for the makepkg-mingw environment? Compiling seems to work without, but one thing they may help with is if MSYS2 build tools are installed, as, at present, they are not. Ruby's compile flags may be the equivalent?

Is there a good reason to update pacman-mirrors in the same line

There have been several changes to it in the last few months. It may be stable now. I don't recall an update stopping due to it.

You should not do pacman -Sy

With images like AppVeyor, that aren't updated often, I would consider that true. With Actions images, that hasn't been an issue. Note that setup-msys2-gcc uses update logic similar to our head builds. If a job fails, it does not replace the current release file.

One important reason for the approach used here.

Generally, many users of RI2 builds update their MSYS2 upon install. Today, ruby-head will always have a current MSYS2. In the future (when Ruby 3.1 is released), the 3.1 RI2 build may have an out of date MSYS2, dependent on how often Ruby patch releases are done.

So, by using a cron job with setup-msys2-gcc, we always have up to date tools. That will not always happen with RI2 installs.

Note that I have no problem with setup-msys2-gcc moving to ruby or rubyinstaller2, or similar functionality being added in either place.

EDIT:

In the future, there could be three 7z files, msys2, mingw64, and ucrt64, updated one or more times daily.

@larskanis
Copy link
Contributor

the environment variables should be set by RubyInstaller

Not sure. Have you checked to see whether the environment variables are needed for general build use, or are they needed for the makepkg-mingw environment?

The environment variables are set because I had several issues without some of them. The last one I remember was for a correct detection of the compiler triplet in a UCRT64 environment by config.guess. Another issue was due to missing LANG setting and one more due to missing PKGCONFIG variable. All issues have been in some configure scripts, but I compiled too many programs and libraries to tell which one it was. It's probably not relevant for MINGW tools, but the MSYS2 tools are Unix tools and they expect some variables.

The other reason is that I think there should be a canonical place where the variable definition should happen. That ensures consistency between the running Ruby and the variables.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Aug 26, 2021

Today the windows-2022 image is available on Actions, and it contains only MSYS2 files, probably for the shell. Also, Ubuntu-16.04 appears to be unavailable.

Anyway, as mentioned above, I think the way to keep CI the fastest is to create 7z files (stored in a GitHub release) for ucrt, mingw64, and maybe mingw32 in another repo, and install them as required. The other repo would run as a cron job, one or more times daily.

If this is done, the next question is whether the 7z files should contain only build tools, or contain additional packages, for instance, the packages needed to build Ruby...

Thoughts?

@larskanis
Copy link
Contributor

larskanis commented Aug 31, 2021

I think the way to keep CI the fastest is to create 7z files (stored in a GitHub release) for ucrt, mingw64, and maybe mingw32 in another repo, and install them as required. The other repo would run as a cron job, one or more times daily.

Yes, I agree with you. I tried to add an option to rubyinstaller to install only MINGW/UCRT tools (as discussed here) and re-use MSYS2 from another path, but MSYS2 is fixated about the path of MINGW tools. Working with links between MSYS2 and MINGW makes things too complicated for being used only in the CI environment. So I think we should proceed with this mechanism.

If this is done, the next question is whether the 7z files should contain only build tools, or contain additional packages, for instance, the packages needed to build Ruby...

As explained, I like to specify dependencies explicit and to fail if not, but that's a matter of opinion.

@MSP-Greg
Copy link
Collaborator Author

Closed via #224

@MSP-Greg MSP-Greg closed this Apr 17, 2022
@MSP-Greg MSP-Greg deleted the win-ucrt branch May 16, 2022 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Windows: Add support for MINGW-UCRT based ruby
3 participants