Skip to content

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented May 12, 2025

This is a fix for the failing test_installation.py on Windows, introduced by #1285. The failures are related to the --target install command, which is not properly recognized on Windows. This pull request reverts the installation command to the previous version, which uses --install for Windows.

example of passed Windows Ninja job:
https://github.com/oneapi-src/unified-memory-framework/actions/runs/14972212328/job/42055589833?pr=1310

and Windows NMake job:
https://github.com/oneapi-src/unified-memory-framework/actions/runs/14972212328/job/42055589878?pr=1310

full nightly run:
https://github.com/oneapi-src/unified-memory-framework/actions/runs/15117728112

@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch from a3dfd44 to 71bfce1 Compare May 12, 2025 09:08
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch 2 times, most recently from 07b22ab to 5f8174a Compare May 12, 2025 12:27
@bratpiorka bratpiorka changed the title todo use --install in installation tests on Windows May 12, 2025
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch from 5f8174a to 429e79e Compare May 12, 2025 13:13
@bratpiorka bratpiorka marked this pull request as ready for review May 12, 2025 13:14
@bratpiorka bratpiorka requested a review from a team as a code owner May 12, 2025 13:14
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch 21 times, most recently from c8bb496 to f9dd35e Compare May 14, 2025 08:54
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch 18 times, most recently from a9e06d2 to b5f178b Compare May 19, 2025 16:00
@bratpiorka
Copy link
Contributor Author

@ldorau @lukaszstolarczuk ready for re-review

@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch from b5f178b to ce17d11 Compare May 20, 2025 06:37
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

in general LGTM

- name: Run tests
working-directory: ${{env.BUILD_DIR}}
run: ctest -C ${{matrix.build_type}} --output-on-failure --test-dir test
# For CMake versions < 3.22 we have to add the build directory to the PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

that's actually super crazy, as I can see the same issue would be with examples... so perhaps we should handle this in CMake...? (warn users? fix the paths already in CMake...? enforce newer versions if test/examples are enabled...?)

// no need to do this in this PR - I can see you marked this as TODO in CMake 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the examples - they are just regular executables, so the user needs to take care of the correct PATH, as usual. This issue is related only with ctest, as it contains some 'magic' that sets up paths and runs the tests. Anyway, if we encounter more problems here, I will create a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we use "ENVIRONMENT_MODIFICATION" in examples' CMake, but well... we don't execute them here, so we should be good for now

@bratpiorka bratpiorka force-pushed the rrudnick_fix_ninja branch 2 times, most recently from 2117a1c to 748c033 Compare May 21, 2025 07:05
@lukaszstolarczuk lukaszstolarczuk merged commit c27abca into oneapi-src:main May 21, 2025
81 of 86 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