Skip to content

Makes Windows build fail earlier with more descriptive error message #298

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.

Sign up for GitHub

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

Open
OlekRaymond wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Makes Windows build fail earlier with more descriptive error message #298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9716520
Makes Windows build fail earlier with more descriptive error message
OlekRaymond Jul 14, 2025
66e52de
Addresses review comments
OlekRaymond Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
*~
*.so
/src/tools/simple-kernel-timer/kp_reader
# Expected build dir
/build
Comment thread
dalg24 marked this conversation as resolved.
Outdated
5 changes: 5 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ cmake_minimum_required(VERSION 3.16 FATAL_ERROR)

project(KokkosTools CXX)

# 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()

Comment on lines +5 to +9
Copy link
Copy Markdown
Contributor

@JBludau JBludau Jul 14, 2025

Choose a reason for hiding this comment

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

Does it even build on windows?

Copy link
Copy Markdown
Author

@OlekRaymond OlekRaymond Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@JBludau JBludau Jul 14, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Author

@OlekRaymond OlekRaymond Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

.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 :)

Copy link
Copy Markdown
Contributor

@JBludau JBludau Jul 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@dalg24 dalg24 Jul 14, 2025

Choose a reason for hiding this comment

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

Where would that variable be defined?
Why is it not sufficient to use KokkosTools_ENABLE_TESTS={ON,OFF}

Copy link
Copy Markdown
Author

@OlekRaymond OlekRaymond Jul 14, 2025

Choose a reason for hiding this comment

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

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. :)

Copy link
Copy Markdown
Author

@OlekRaymond OlekRaymond Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@masterleinad masterleinad Jul 14, 2025

Choose a reason for hiding this comment

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

I'd suggest to add the option in a pull request that would actually add some Windows support.

Copy link
Copy Markdown
Author

@OlekRaymond OlekRaymond Jul 15, 2025

Choose a reason for hiding this comment

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

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)

if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 17)
endif()
Expand Down