-
Notifications
You must be signed in to change notification settings - Fork 21
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 CI build #168
Windows CI build #168
Conversation
2b38634
to
c62e737
Compare
bf86bc8
to
5f3579a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TomMelt
Looks sensible to me (as much as any windows code does...)
I think we need some documentation adding about how to run the test suite for Windows users (definitely online and maybe README, though perhaps online only to avoid pollution)
I know nothing about bat scripts, but is there a reason we apply it in a for loop for each example as part of the workflow rather than running all examples from the bat script as we do in run_integration_tests.sh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting in the effort to get this working @TomMelt. Just a few minor comments from me.
a005817
to
6ec89a0
Compare
5f3579a
to
cfec44f
Compare
fixed by a6ec9c2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TomMelt This looks great, I think we just need the online documentation updating to match (in pages/
- definitely testing.md
, but worth a quick scan of anywhere else, including the Windows specific docs).
|
fe20cbf
to
b6650f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TomMelt
This is really great, and a significant hurdle - hopefully you learnt something useful, even if it is at the cost of becoming the "Windows guy". ;P
The blocker for me is not only just to bump the version of CMake required by FTorch to be 3.15 (for use of the Python VIRTUALENV) feature (see specific comment).
Consider also updating in the example CMakeLists for consistency, and in the documentation. From past experience the easiest way to do this is to grep the full source for 3.5
.
Once that is done I am happy to approve.
Other question is whether there is anything you feel is particularly useful or unintuitive to highlight for Windows users within the workflow. Otherwise we can just update the Windows docs piece by piece as people raise things in future.
src/CMakeLists.txt
Outdated
@@ -101,6 +101,10 @@ install(FILES "${CMAKE_BINARY_DIR}/modules/ftorch_test_utils.mod" | |||
|
|||
# Build integration tests | |||
if(CMAKE_BUILD_TESTS) | |||
|
|||
set (Python3_FIND_VIRTUALENV FIRST) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a CMake 3.15 feature, so perhaps we need to bump the dependency across FTorch to CMake 3.15?
I think this is still sufficiently old not to be an issue for any users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 17d8db2
Thanks @jatkinson1000 , I have added a concrete example to the online docs just in case anyone has difficulty (most likely my future self) |
e7ebd4d
to
56b34c8
Compare
b233133
to
d3d8865
Compare
d3d8865
to
e82816e
Compare
56b34c8
to
b97b758
Compare
Thanks @TomMelt I think README.md and /pages/cmake.md still need the CMake versioin updating from 3.5 to 3.15. |
b97b758
to
b3b0e12
Compare
b3b0e12
to
ae23c9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this now.
Thanks @TomMelt it has been a mammoth task you've done here!
This PR adds windows CI running on latest version of windows with intel compilers version
2023.2
.For green computing reasons I have only prepared an intel build. My plan is that we have:
gcc
on linuxclang
on mac; andintel
on windowsThat way we can get nice coverage without hammering compute resources.