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: try UCRT for windows ruby-head builds #2272

Merged
merged 4 commits into from
Jun 19, 2021

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

The 3.1.0.dev windows ruby installer uses UCRT, see #2268 for context. This gives Nokogiri and @larskanis a feedback loop.

@flavorjones
Copy link
Member Author

@larskanis This looks like it might be a genuine failure on UCRT?

https://github.com/sparklemotion/nokogiri/runs/2848463493?check_suite_focus=true#step:7:50

@larskanis
Copy link
Member

The failure happens when calling the XLST function date:date(). So the issue resides in libxslt pretty sure. In contrast to MSVCRT, UCRT has non ANSI-C compatible time functions. This already needed a fix in ffi. I guess that libxslt simply disables date+time functions on UCRT. I'll try to fix that.

@flavorjones
Copy link
Member Author

flavorjones commented Jun 17, 2021

@larskanis That's interesting! Looking at libexslt/date.c it seems like WITH_TIME is a compile-time macro:

#if defined(HAVE_TIME_H)					\
    && (defined(HAVE_LOCALTIME) || defined(HAVE_LOCALTIME_R))	\
    && (defined(HAVE_GMTIME) || defined(HAVE_GMTIME_R))		\
    && defined(HAVE_TIME)
#define WITH_TIME
#endif

I'll also note that it's fine when Nokogiri compiles it (--disable-system-libraries), but not when we use the UCRT system libraries (--enable-system-libraries), which I don't fully understand.

Anything I can do to help?

@flavorjones
Copy link
Member Author

Also: I'd be fine with not fixing this. For example, when using a version of libxml2 that does not have iconv support compiled in, we skip the tests and expose that configuration as part of VERSION_INFO.

@flavorjones
Copy link
Member Author

WDYT of the following solution:

  • during Nokogiri initialization, call exsltDateDate(NULL)
  • if the return value is NULL, set a ruby constant like NOKOGIRI_LIBXSLT_TIME_SUPPORT to false
  • else set it to true
  • expose it in VERSION_INFO

Then we can either rewrite the failing test to not depend on date/time functionality, and/or conditionally run tests based on the value of the constant.

@larskanis
Copy link
Member

I opened a PR on Mingw-packages addressing the date/time issue: msys2/MINGW-packages#8957

@flavorjones
Copy link
Member Author

@larskanis In the meantime, I have a commit on this PR to detect whether libxslt is built with date-time functionality, and skip the assertion if not.

@flavorjones flavorjones force-pushed the flavorjones-ci-ucrt-windows-builds branch 3 times, most recently from 973b813 to a7fde5c Compare June 19, 2021 01:28
flavorjones and others added 4 commits June 18, 2021 21:41
see #2268 for context

Co-authored-by: MSP-Greg <[email protected]>
- expose this compile-time config as Nokogiri::VERSION_INFO["libxslt"]["datetime_enabled"]
- skip the assertion that depends on this behavior
@flavorjones flavorjones force-pushed the flavorjones-ci-ucrt-windows-builds branch from a7fde5c to 5bb0bcb Compare June 19, 2021 01:41
@flavorjones flavorjones merged commit 1508a66 into main Jun 19, 2021
@flavorjones flavorjones deleted the flavorjones-ci-ucrt-windows-builds branch June 19, 2021 02:43
@flavorjones flavorjones added this to the v1.12.0 milestone Jun 19, 2021
@larskanis
Copy link
Member

The latest MSYS2 package mingw-w64-ucrt-x86_64-libxslt-1.1.34-4 is now in the official repository and it fixes the date/time issue on UCRT64.

@flavorjones
Copy link
Member Author

flavorjones commented Jun 21, 2021

@ghost

This comment was marked as off-topic.

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.

2 participants