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

CI: Add UCRT based test on Appveyor #2268

Closed
wants to merge 2 commits into from
Closed

CI: Add UCRT based test on Appveyor #2268

wants to merge 2 commits into from

Conversation

larskanis
Copy link
Member

and use a wider range of ruby versions

What problem is this PR intended to solve?

I would like to test nokogiri on rubyinstaller-head which has switched to a newer C runtime called UCRT. Unfortuantely https://github.com/MSP-Greg/setup-ruby-pkgs doesn't support UCRT yet, so that I reactivated appveyor x64 build for it. While being over there, I would suggest to use a wider range of ruby versions, so that the latest and newest supported version is checked.

Have you included adequate test coverage?

The CI run.

Does this change affect the behavior of either the C or the Java implementations?

Only C.

and use a wider range of ruby versions
Which isn't currently on Appveyor's MINGW-UCRT.
@flavorjones
Copy link
Member

@larskanis is this why ruby-head on windows is failing at the moment? (example: https://github.com/sparklemotion/nokogiri/runs/2837044316?check_suite_focus=true)

@flavorjones
Copy link
Member

@larskanis also, we're testing every version of Ruby on windows:

image

What versions were you thinking of when you say " I would suggest to use a wider range of ruby versions"?

@flavorjones
Copy link
Member

@larskanis One other note, in addition to my comments above: the real value that the appveyor builds are providing right now (given that we're testing windows thoroughly in github actions) is the 32-bit windows builds, which have been removed in this PR. If you'd like to add UCRT coverage on ruby-head, that's great. But please restore the 32-bit tests.

Is there a plan to work with @MSP-Greg on UCRT support on Github Actions?

@MSP-Greg
Copy link
Contributor

I just opened an issue re adding the ucrt packages to the Actions Windows images, and I'll be looking at the code in setup-ruby and setup-ruby-packages. So, yes, we're working on it...

@larskanis
Copy link
Member Author

@flavorjones Sorry for the noise - the CI didn't run without opening a pull request. But now that it run, I noticed the (failing) test run in the "upstream" github action, that you already mentioned above.

So since there is already a rubyinstaller-head test run in the CI, so this PR can be considered obsolete. The reason why the "upstream" test fails, is that the wrong PATH is set and the wrong packages are installed. The PATH should be
C:/msys64/ucrt64/bin/
instead of
C:/msys64/mingw64/bin/
and the package should be
mingw-w64-ucrt-x86_64-libxslt
instead of
mingw-w64-x86_64-libxslt
.

However both issues should be solved in https://github.com/MSP-Greg/setup-ruby-pkgs .

@larskanis
Copy link
Member Author

" I would suggest to use a wider range of ruby versions"?

What I meant is, that ruby-2.7 and 3.0 are close together, while ruby-2.4 and ruby-head might trigger more potential issues on the 32 bit x86 platform.

@flavorjones
Copy link
Member

@MSP-Greg Thanks for opening that issue!

@larskanis Thanks for the additional context! I'll close this, but will also take your advice on adding versions for the 32-bit build (I was being lazy).

@MSP-Greg
Copy link
Contributor

@larskanis

It's not quite that simple. The path manipulation happens in setup-ruby, not setup-ruby-pkgs. I think setup-ruby can be patched to set the correct path (without a breaking change), and same for patching setup-ruby-pkgs.

Currently on Windows, setup-ruby always enables the mingw paths for access to the tools. For mswin and older MSYS Rubies, it additionally adds the 'paths' for their tools.

To clarify, setup-ruby-pkgs is designed to simplify cross-platform package installation and updates. If that isn't needed or desired, setup-ruby is all that is needed. But, setup-ruby also assumes that packages/tools are installed on the Actions image, and currently the ucrt packages/tools are not.

Also, setup-ruby-pkgs uses the code in setup-ruby. So, if we change the code in setup-ruby to set the correct path, and I also change the code in setup-ruby-pkgs to install ucrt packages with ruby-head, it will work for any people using setup-ruby-pkgs.

But, if they're only using setup-ruby, it will break because none of the packages are installed.

Hence, let's wait and see the response to adding ucrt to the Actions image? Regardless, I'll get the code ready.

@MSP-Greg
Copy link
Contributor

@flavorjones & @larskanis

I added Windows ruby-head (the ucrt build) to Actions.

Please see https://github.com/MSP-Greg/nokogiri/actions/runs/944724538

windows (enable, head) compiled with a lot of warnings, failed one test.

windows (disable, head) also compiled with a lot of warnings, but passed all tests.

Feel free to look at the test logs.

As to the code, I've got debug output in it, etc. It involves patches to both setup-ruby and setup-ruby-pkgs, and since there are no ucrt packages installed on the Actions image, it is updating a lot more that needed, and it isn't setup to do that on a particular job, it does so for all.

Or, maybe we can wait to see if MSFT/GitHub will be adding the ucrt packages...

flavorjones added a commit that referenced this pull request Jun 17, 2021
see #2268 for context

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
@flavorjones
Copy link
Member

@MSP-Greg Thanks for that! I've posted a branch that will build using those tags at #2272

flavorjones added a commit that referenced this pull request Jun 19, 2021
see #2268 for context

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
flavorjones added a commit that referenced this pull request Jun 19, 2021
see #2268 for context

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
@flavorjones flavorjones deleted the ucrt branch August 19, 2021 20:59
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.

None yet

3 participants