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

[ML] Portability improvements #2313

Merged

Conversation

edsavage
Copy link
Contributor

Replace calls to unix like commands with either CMake commands or the
appropriate command for the native platform.

Relates #2311

Replace calls to unix like commands with either CMake commands or the
appropriate command for the native platform.

Relates elastic#2311
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

The direction of travel is great.

I just commented on a few low level details.

3rd_party/pull-eigen.cmake Outdated Show resolved Hide resolved
3rd_party/pull-eigen.cmake Outdated Show resolved Hide resolved
cmake/compiler/vs2019.cmake Outdated Show resolved Hide resolved
cmake/compiler/vs2019.cmake Show resolved Hide resolved
cmake/variables.cmake Outdated Show resolved Hide resolved
lib/ver/CMakeLists.txt Outdated Show resolved Hide resolved
lib/ver/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@edsavage
Copy link
Contributor Author

retest

edsavage added 2 commits June 20, 2022 09:54
Pulling Eigen at build time is problematic for parallel build as it
introduces a race condition. The safer option is to pull at
configuration time.
Remove the call to the eigen target in the build.gradle file as it no
longer exists
@edsavage edsavage merged commit 1a6b198 into elastic:feature/cmake_build Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants