Makes Windows build fail earlier with more descriptive error message #298
Conversation
| # Users cannot use Kokkos tools on windows, we want to test things on windows/msvc however | ||
| if(WIN32 AND NOT DEFINED KOKKOS_TOOLS_TEST_WINDOWS) | ||
| message(FATAL_ERROR "Kokkos tools is not supported on Windows, please attempt to profile on expected hardware") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Does it even build on windows?
There was a problem hiding this comment.
Yes,
The libs all build to completion, the .so objects are skipped and the build can pass successfully (all require certain cmake options)
At the moment you'd have to read the documentation to figure out that it's not supported* , which is fine but lots of people don't so it's easier to just
* The user has to find out how to use it, find out that requires .sos then find out the sos don't build on windows (through a cmake warning) so this just makes it more clear
There was a problem hiding this comment.
Maybe I don't understand ... but isn't .so the ending linux uses for shared objects? for windows it should be .dll, right? Can the resulting shared object be used on windows or are they broken?
There was a problem hiding this comment.
.so the ending Linux uses for shared objects? for windows it should be .dll,
Yes they are the same concept and solve the same problems but the syntax and lookup changes between the two.
Can the resulting shared object be used on window
No, In short we need some (minimal) code changes to make the .dll useful. I am considering doing this but I expect would see little user interaction because they should be profiling on expected hardware (like HPCs) which is rarely running Windows.
Full disclosure: I work for a consultancy which has customers that like Windows, I'm not just a Linux hater :)
There was a problem hiding this comment.
Yeah, I think the tools don't really have windows users on windows machines. Do you have seen them be used on WSL?
If the tools don't work on windows, maybe we should just have CMake error out
There was a problem hiding this comment.
Where would that variable be defined?
Why is it not sufficient to use KokkosTools_ENABLE_TESTS={ON,OFF}
There was a problem hiding this comment.
them be used on WSL
I remember reading (I don't know if it was Kokkos docs or other docs) not to profile in non-target environments so I think it's moot. I see no reason you can't profile a Kokkos executable built in WSL from WSL though.
we should just have CMake error out
I agree, (with no intention to be rude) that is the point of the ticket. :)
There was a problem hiding this comment.
Where would that variable be defined?
I has to be given in the CMake invocation. i.e. cmake -B something -S . -DKOKKOS_TOOLS_TEST_WINDOWS=ON
Why is it not sufficient to use KokkosTools_ENABLE_TESTS={ON,OFF}
I wanted something unique:
- so a user is unlikely to enable it by mistake and not see the error message.
- a dev is less likely to enable it
- since no devs are working on windows it won't be seen as excessive.
- it is unlikely to have any effect on a pipeline or future commits
- it does not clutter the help dialog in cmake like an option would as it is not intended for users
There was a problem hiding this comment.
I'd suggest to add the option in a pull request that would actually add some Windows support.
There was a problem hiding this comment.
I did think about that but it makes all the other checks for windows moot
I think it's fine here for now (does no harm and isn't particularly invasive)
4 participants
Building on windows is not currently supported but this could be communicated better.
I have made a change that makes the windows build fail immediately with an appropriate error message in the CMake. All other checks for Windows have been left alone so that Windows support can be added at a later time.
Ticket came as an improvement to pull request #289 which has been rendered obsolete
Thank you