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

More windows-related fixes #126

Merged
merged 9 commits into from
Oct 11, 2024

Conversation

mortbopet
Copy link
Contributor

@mortbopet mortbopet commented Oct 10, 2024

  • use ! instead of a not macro
  • Don't build the planparser tool on windows; this uses unix commands to parse arguments.
  • std::filesystem::path doesn't implicitly convert to std::string on msvc
  • A shared library must specify export symbols (or set CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS) for a .lib file to be generated, which is required for things to link against said library.
  • Can't use * (wildcard) on windows in a CMake command; use built-in CMake globbing instead.
  • substrait-cpp tags an ancient version of protobuf (23.4) which itself uses an ancient version of abseil, that fails to build on MSVC. Bump protobuf to the newest version (28.2).
  • various nitty changes related to the protobuf bump

Also requires EpsilonPrime/protobuf-matchers@91968ca to land.

- use `!` instead of a `not` macro
- Don't build the `planparser` tool on windows; this uses unix commands to parse arguments.
- std::filesystem::path doesn't implicitly convert to std::string on msvc
- A shared library must specify export symbols (or set `CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS`) for a .lib file to be generated, which is required for things to link against said library.
- Can't use `*` (wildcard) on windows in a CMake command; use built-in CMake globbing instead.
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.

I'm fine with all these changes.

Of the tools available the plantransformer is probably the only one I truly use regularly (the planparser and planconverter were more useful during development). Maybe I'll make those optional in the same way tests are.

@mortbopet
Copy link
Contributor Author

@EpsilonPrime Awesome... might need some help with this PR wrt. getting CI to pass. Build/test is passing, but for some reason, check is failing.

@mbrobbel
Copy link
Member

@EpsilonPrime Awesome... might need some help with this PR wrt. getting CI to pass. Build/test is passing, but for some reason, check is failing.

Looks like the runner is getting killed. Maybe it's running out of memory? Can you try to reduce the parallelism here:

pushd tmp/src/substrait/proto && make -j && popd || exit
?

@mortbopet
Copy link
Contributor Author

@mbrobbel thanks for the tip - reduced to -j 2, and things now pass :).

@EpsilonPrime EpsilonPrime merged commit 904a493 into substrait-io:main Oct 11, 2024
3 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.

3 participants