Skip to content

Conversation

@arixmkii
Copy link
Contributor

@arixmkii arixmkii commented Apr 23, 2025

On Windows both '\' and '/'` would be treated as path separators - need to replace both.

Additional changes - removed assertion, where error is not actually updated - no point in asserting previous error multiple times.

Before this change running Unit tests would result in

=== RUN   TestConvertToRaw
=== RUN   TestConvertToRaw/qcow_without_backing_file
time="2025-04-23T21:27:30+03:00" level=info msg="Converting \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\qcow.img\" (qcow2) to a raw disk \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\TestConvertToRaw\\\\qcow_without_backing_file\""
    nativeimgutil_test.go:68: assertion failed: error is not nil: open C:\msys64\tmp\TestConvertToRaw1483052598\001\TestConvertToRaw\qcow_without_backing_file.lima-1923521838.tmp: The system cannot find the path specified.
=== RUN   TestConvertToRaw/qcow_with_backing_file
time="2025-04-23T21:27:30+03:00" level=info msg="Converting \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\qcow.img\" (qcow2) to a raw disk \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\TestConvertToRaw\\\\qcow_with_backing_file\""
    nativeimgutil_test.go:77: assertion failed: error is not nil: open C:\msys64\tmp\TestConvertToRaw1483052598\001\TestConvertToRaw\qcow_with_backing_file.lima-831870062.tmp: The system cannot find the path specified.
=== RUN   TestConvertToRaw/qcow_with_extra_size
time="2025-04-23T21:27:30+03:00" level=info msg="Converting \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\qcow.img\" (qcow2) to a raw disk \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\TestConvertToRaw\\\\qcow_with_extra_size\""
    nativeimgutil_test.go:86: assertion failed: error is not nil: open C:\msys64\tmp\TestConvertToRaw1483052598\001\TestConvertToRaw\qcow_with_extra_size.lima-3723446434.tmp: The system cannot find the path specified.
=== RUN   TestConvertToRaw/raw
time="2025-04-23T21:27:30+03:00" level=info msg="Converting \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\raw.img\" (raw) to a raw disk \"C:\\\\msys64\\\\tmp\\\\TestConvertToRaw1483052598\\\\001\\\\TestConvertToRaw\\\\raw\""
    nativeimgutil_test.go:95: assertion failed: error is not nil: failed to copy "C:\\msys64\\tmp\\TestConvertToRaw1483052598\\001\\raw.img" into "C:\\msys64\\tmp\\TestConvertToRaw1483052598\\001\\TestConvertToRaw\\raw": failed to open target C:\msys64\tmp\TestConvertToRaw1483052598\001\TestConvertToRaw\raw: open C:\msys64\tmp\TestConvertToRaw1483052598\001\TestConvertToRaw\raw: The system cannot find the path specified.
--- FAIL: TestConvertToRaw (0.11s)
    --- FAIL: TestConvertToRaw/qcow_without_backing_file (0.00s)
    --- FAIL: TestConvertToRaw/qcow_with_backing_file (0.00s)
    --- FAIL: TestConvertToRaw/qcow_with_extra_size (0.00s)
    --- FAIL: TestConvertToRaw/raw (0.00s)
=== RUN   FuzzConvertToRaw
--- PASS: FuzzConvertToRaw (0.00s)
FAIL
FAIL    github.com/lima-vm/lima/pkg/nativeimgutil       0.902s

On Windows both '\' and '/'` would be treated as path separators - need to
replace both.

Signed-off-by: Arthur Sengileyev <[email protected]>
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

But this PR does not seem to address the current Windows failure on GitHub CI.

I would prefer to see that fixed before we merge more Windows changes that currently would not be checked by CI.

@arixmkii
Copy link
Contributor Author

arixmkii commented Apr 23, 2025

@jandubois this is a different one. I'm still looking into Integration test issue. I plan to address that as well.

I'm fine with this being merged at a later time. 👍 I created it so I would not forget about this one.

@jandubois
Copy link
Member

I'm fine with this being merged at a later time. 👍 I created it so I would not forget about this one.

Thanks, sounds good. Please rebase this PR on master once the integration test is fixed and merged, so we can see if the Windows tests remain green with this change (it all looks fine to me, but I don't want us to get into the habit of merging PRs unless CI is all green).

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit ef25b90 into lima-vm:master Apr 24, 2025
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants