-
Notifications
You must be signed in to change notification settings - Fork 37
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
use --install in installation tests on Windows #1310
Conversation
a3dfd44
to
71bfce1
Compare
07b22ab
to
5f8174a
Compare
5f8174a
to
429e79e
Compare
c8bb496
to
f9dd35e
Compare
a9e06d2
to
b5f178b
Compare
@ldorau @lukaszstolarczuk ready for re-review |
b5f178b
to
ce17d11
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.
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 |
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.
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 👍
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 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
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 think we use "ENVIRONMENT_MODIFICATION" in examples' CMake, but well... we don't execute them here, so we should be good for now
2117a1c
to
748c033
Compare
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