ci: add WSL2 CI #1883

pendo324 · 2023-10-04T19:31:00Z

Picking up where #1826 left off

According to these docs, Windows runners should have Git for Windows pre-installed, meaning tar.exe and gzip.exe should be in the path already.

Let's see if just changing the rootfs from zstd to gz compression gets us any further, and iterate from there.

Closes #1781.

pendo324 · 2023-10-04T21:51:30Z

See a weird Forbidden (403). error, from wsl --update I think? Going to retry it in a few minutes to see if its a network issue

pendo324 · 2023-10-04T23:53:12Z

Well, I found out why the error wasn't being logged, fixed in 88ce268.

The wsl command (or subcommand wslpath command) was returning an error that implemented ExitCode(), which caused the final logrus.Fatal to never get executed.

I also added some quotes to wrap the input of the temp file path when calling wslpath.

Now it seems like the output of wslpath is just... wrong for some reason, outputting:

time="2023-10-04T23:45:50Z" level=info msg="[hostagent] limaBootFilePathOnLinux: \"C:UsersRUNNER~1AppDataLocalTemplima-wsl2-boot-36860512[26](https://github.com/lima-vm/lima/actions/runs/6412733028/job/17410508491?pr=1883#step:8:27).sh\"\n"

From my local testing, double backslashes seem to work. Adding a change to convert \ to \\

balajiv113 · 2023-10-06T08:13:48Z

In WSL2, all folders are shared under C drive ??

I could see script is under C:/Users/RUNNER~1/AppData/Local/Temp/ and userhome under C:\\\\Users\\\\runneradmin\\\\.lima\\\\wsl2

pendo324 · 2023-10-06T18:12:00Z

In WSL2, all folders are shared under C drive ??

I could see script is under C:/Users/RUNNER~1/AppData/Local/Temp/ and userhome under C:\\\\Users\\\\runneradmin\\\\.lima\\\\wsl2

Yeah, the entire C drive is shared by default.

I'm pretty sure the files exist, but the wslpath call isn't returning the right output. I'm going to try to debug it on my local windows machine as to not waste resources. Just running the expected command (wsl.exe -d <distro> wslpath -u "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\lima-wsl2-boot-2835403810.sh") does work and outputs a valid WSL path for me on my machine in a PowerShell terminal, so I'm not sure why its not working here.

balajiv113 · 2023-10-06T18:20:18Z

If this still doesn't work, we can compute the wslpath ourself (/mnt prefix and slashes changed accordingly)
There might already be a go library as well to do this.

I hope this should surely work without these glitches

jandubois · 2023-10-06T18:49:37Z

I don't understand why this is necessary:

bootFileWSLPath := strconv.Quote(limaBootFilePathOnWindows)

Isn't exec.Command() already taking care of the quoting? So you would now pass in a double quoted string?

we can compute the wslpath ourself.

You can't really, in general, as even the mount prefix is configurable.

pendo324 · 2023-10-06T21:02:18Z

I don't understand why this is necessary:
bootFileWSLPath := strconv.Quote(limaBootFilePathOnWindows)
Isn't exec.Command() already taking care of the quoting? So you would now pass in a double quoted string?

I used it to convert the slashes from \ to \\ and add the quote around the path string, in case it has a space in it. It seems like wslpath expects \\ path separators, at least on certain versions of WSL.

From my testing, the command only works as intended when going through bash -c. I'd be lying if I said I understood exactly why this is the case 🤷‍♂️, but it seems to at least output a reasonable path now, so yay (if anyone has an explanation, I'm very interested).

Seems like WSL still might have an issue with the ~ in the path from the looks of it. Seems like that's from an "8.3" or "short" filename, looking in to how to convert that to the "long" filename to see if that helps

The short file name in question is the user directory:
C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\lima-wsl2-boot-3366004968.sh, which does at least get "properly" converted to /mnt/c/Users/RUNNER~1/AppData/Local/Temp/lima-wsl2-boot-3366004968.sh by wslpath now when checking the run logs.

jandubois · 2023-10-06T22:02:37Z

looking in to how to convert that to the "long" filename

I think the short names should just work, even from WSL.

You can convert them with PowerShell, but that has a high startup cost. But if you only do it once, then it doesn't matter:

C:\>powershell "(Get-Item -LiteralPath 'C:\PROGRA~1').FullName"
C:\Program Files

jandubois · 2023-10-06T22:11:54Z

I'd be lying if I said I understood exactly why this is the case

The problem is that Windows doesn't have the concept of passing *argv[] to a new process; all you get is a single char*. So every app (or runtime) implements their own commandline parsing and argument quoting rules. It is all a big mess.

command.exec() has to smash the arguments together on its own, quoting them in a way that most programs will be able to parse again. But it is all based on heuristics.

pendo324 · 2023-10-07T00:30:51Z

We have liftoff :)

pendo324 · 2023-10-07T00:39:55Z

looking in to how to convert that to the "long" filename

I think the short names should just work, even from WSL.

You can convert them with PowerShell, but that has a high startup cost. But if you only do it once, then it doesn't matter:
C:\>powershell "(Get-Item -LiteralPath 'C:\PROGRA~1').FullName"
C:\Program Files

Yeah you're right, the short path actually does work with wslpath. I added the code to convert to a regular "long path", using syscalls instead of PowerShell, but I'll probably remove it before merging unless there's a good reason to keep it

AkihiroSuda · 2023-10-07T23:16:25Z

Seems green now 💚

Please rebase, add back macOS CI, and ping me when this is ready to merge

pendo324 · 2023-10-09T18:32:14Z

@AkihiroSuda, I think it's good now. Let me know if you want me to make any more changes

AkihiroSuda · 2023-10-09T18:37:48Z

.github/workflows/test.yml

+        # otherwise `wsl --list --verbose` (called from Lima) fails:
+        # https://github.com/lima-vm/lima/pull/1826#issuecomment-1729993334
+        # The distro image itself is not consumed by Lima.
+        # ------------------------------------------------------------------


I’d like to see this resolved. Can be another PR though.

I'll keep looking into this separately. Maybe there's something we can do with conditionally calling wsl --import / wsl --install based on the status returned from wsl --list -v

AkihiroSuda · 2023-10-09T18:38:12Z

pkg/hostagent/hostagent.go

@@ -378,6 +380,7 @@ func (a *HostAgent) Run(ctx context.Context) error {
 		}()
 		return a.driver.RunGUI()
 	}
+	logrus.Infof("Running startRoutinesAndWait")


Should be Debugf, or just removed

AkihiroSuda · 2023-10-09T19:54:00Z

pkg/hostagent/hostagent.go

 	// WSL instance SSH address isn't known until after VM start
 	if *a.y.VMType == limayaml.WSL2 {
 		sshAddr, err := store.GetSSHAddress(a.instName)
+		logrus.Infof("sshAddress: %s", sshAddr)


Debugf or remove

AkihiroSuda · 2023-10-09T19:54:11Z

pkg/hostagent/hostagent.go

@@ -316,9 +316,11 @@ func (a *HostAgent) Run(ctx context.Context) error {
 		return err
 	}

+	logrus.Infof("After driver.Start()")


Debugf or remove

Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Justin Alvarez <[email protected]> Co-authored-by: Akihiro Suda <[email protected]>

AkihiroSuda

Thanks 💚

AkihiroSuda added this to the v0.18.0 milestone Oct 4, 2023

AkihiroSuda added area/test Tests and CI platform/Windows labels Oct 4, 2023

AkihiroSuda marked this pull request as draft October 7, 2023 23:16

pendo324 force-pushed the wsl2-ci branch 2 times, most recently from 1359538 to 914a5ac Compare October 9, 2023 17:49

AkihiroSuda reviewed Oct 9, 2023

View reviewed changes

pendo324 force-pushed the wsl2-ci branch from 914a5ac to 0a7e969 Compare October 9, 2023 19:50

AkihiroSuda reviewed Oct 9, 2023

View reviewed changes

ci: add WSL2 CI

7dda143

Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Justin Alvarez <[email protected]> Co-authored-by: Akihiro Suda <[email protected]>

pendo324 force-pushed the wsl2-ci branch from 0a7e969 to 7dda143 Compare October 9, 2023 20:32

AkihiroSuda marked this pull request as ready for review October 9, 2023 21:30

AkihiroSuda approved these changes Oct 9, 2023

View reviewed changes

AkihiroSuda merged commit 7493e1d into lima-vm:master Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: add WSL2 CI #1883

ci: add WSL2 CI #1883

pendo324 commented Oct 4, 2023

pendo324 commented Oct 4, 2023

pendo324 commented Oct 4, 2023 •

edited

Loading

balajiv113 commented Oct 6, 2023

pendo324 commented Oct 6, 2023 •

edited

Loading

balajiv113 commented Oct 6, 2023

jandubois commented Oct 6, 2023

pendo324 commented Oct 6, 2023 •

edited

Loading

jandubois commented Oct 6, 2023

jandubois commented Oct 6, 2023

pendo324 commented Oct 7, 2023

pendo324 commented Oct 7, 2023

AkihiroSuda commented Oct 7, 2023

pendo324 commented Oct 9, 2023

AkihiroSuda Oct 9, 2023

pendo324 Oct 9, 2023

AkihiroSuda Oct 9, 2023

AkihiroSuda Oct 9, 2023

AkihiroSuda Oct 9, 2023

AkihiroSuda left a comment

ci: add WSL2 CI #1883

ci: add WSL2 CI #1883

Conversation

pendo324 commented Oct 4, 2023

pendo324 commented Oct 4, 2023

pendo324 commented Oct 4, 2023 • edited Loading

balajiv113 commented Oct 6, 2023

pendo324 commented Oct 6, 2023 • edited Loading

balajiv113 commented Oct 6, 2023

jandubois commented Oct 6, 2023

pendo324 commented Oct 6, 2023 • edited Loading

jandubois commented Oct 6, 2023

jandubois commented Oct 6, 2023

pendo324 commented Oct 7, 2023

pendo324 commented Oct 7, 2023

AkihiroSuda commented Oct 7, 2023

pendo324 commented Oct 9, 2023

AkihiroSuda Oct 9, 2023

Choose a reason for hiding this comment

pendo324 Oct 9, 2023

Choose a reason for hiding this comment

AkihiroSuda Oct 9, 2023

Choose a reason for hiding this comment

AkihiroSuda Oct 9, 2023

Choose a reason for hiding this comment

AkihiroSuda Oct 9, 2023

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

pendo324 commented Oct 4, 2023 •

edited

Loading

pendo324 commented Oct 6, 2023 •

edited

Loading

pendo324 commented Oct 6, 2023 •

edited

Loading