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 build & test CI and fixes #134

Merged
merged 22 commits into from
Mar 3, 2025

Conversation

mortbopet
Copy link
Contributor

@mortbopet mortbopet commented Feb 28, 2025

find_vs.ps1 is used to enter a visual studio development environment, which in turn, makes ninja available.

This also includes a fix which i've been running with locally for a while... For some yet unknown reason, the googletest dependency within ANTLR4 (antlr4->googletest) applies incorrect assumptions about include path inside the gmock project. This only happens when building with MSVC. We can hot-fix this by injecting the correct paths to the target, after it's been defined.

Unfortunately, there's a lingering issue here where a std::any_cast exception occurs. I've spent a fair bit of time trying to track down the issue, but to no avail. I don't plan on using the textual format any time soon, so for the sake of progress, i've disabled the text-format related tests for now. After merging this PR, I'll create an issue to track it, in case some Windows user stumbles upon this in the future.

@mortbopet mortbopet force-pushed the dev/mpetersen/windows_ci branch 3 times, most recently from 9c6174d to e91f219 Compare February 28, 2025 08:51
@mortbopet mortbopet force-pushed the dev/mpetersen/windows_ci branch from e91f219 to 216c6f4 Compare February 28, 2025 08:53
@EpsilonPrime
Copy link
Member

I think I updated the require rule to the new name so next time I hope it updates the checks appropriately.

@mortbopet mortbopet force-pushed the dev/mpetersen/windows_ci branch from 844e39c to 8535d29 Compare February 28, 2025 09:27
@mortbopet
Copy link
Contributor Author

Currently blocked by antlr/antlr4#4738.

@mortbopet
Copy link
Contributor Author

@EpsilonPrime this PR also bumps the protobuf version to 29.3, required due to another MSVC issue that removed a definition for NAN - which subsequently has been fixed in protobuf at a later tag than what we're currently pointing at.

Let me know if you want that change factored out into a separate PR.

@EpsilonPrime
Copy link
Member

I'm okay keeping it here if it passes. The new version doesn't appear to be negatively affecting the non-windows builds.

Windows needs to a-priori know how to open a file (binary or text). If not, text-mode is assumed, which then will translate line endings `\n->\r\n`. This, in turn, will break the protobuf loader. This in turn means that windows can't rely on `loadPlan`s format detection, in case the underlying file is a binary file.

This change adds adds a `forceBinary` flag to `loadPlan`, which must be used on Windows when providing binary files.

`loadPlan` is used a few different places, and this doesn't pipe that argument in everywhere. Fixes have been added to an extent s.t., tests pass.
@mortbopet
Copy link
Contributor Author

@EpsilonPrime Marking this as ready for review. Windows seems to be in a pretty good spot, with most tests passing.

Unfortunately, there's a lingering issue here where a std::any_cast exception occurs. I've spent a fair bit of time trying to track down the issue, but to no avail. I don't plan on using the textual format any time soon, so for the sake of progress, i've disabled the text-format related tests for now. After merging this PR, I'll create an issue to track it, in case some Windows user stumbles upon this in the future.

@mortbopet mortbopet marked this pull request as ready for review March 3, 2025 13:12
@mortbopet mortbopet requested a review from westonpace as a code owner March 3, 2025 13:12
@mortbopet mortbopet changed the title Add windows job to CI build & test Windows build & test CI and fixes Mar 3, 2025
Copy link
Member

@EpsilonPrime EpsilonPrime left a comment

Choose a reason for hiding this comment

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

There are definitely some tricks to deal with any_cast errors.

@EpsilonPrime EpsilonPrime enabled auto-merge (squash) March 3, 2025 18:05
@EpsilonPrime EpsilonPrime disabled auto-merge March 3, 2025 18:08
@EpsilonPrime EpsilonPrime merged commit 30106bd into substrait-io:main Mar 3, 2025
4 checks passed
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