Skip to content

use --install in installation tests on Windows #1310

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

Merged
merged 1 commit into from
May 21, 2025

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

@@ -318,10 +345,23 @@ jobs:

- 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