Skip to content
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

Create SandboxTest script #827

Merged
merged 39 commits into from
May 3, 2021
Merged

Create SandboxTest script #827

merged 39 commits into from
May 3, 2021

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented May 22, 2020

PS: check this comment if you want to use it before it gets merged.

Description

A new PowerShell script to test the installation of any Manifest in Windows Sandbox.

Usage

$ Get-Help .\Tools\SandboxTest.ps1
SandboxTest.ps1 [[-Manifest] <string>] [[-Script] <scriptblock>] [-MapFolder <string>] [<CommonParameters>]

Examples

$ .\Tools\SandboxTest.ps1 .\Tools\SandboxTest.ps1 .\manifests\GitHub\cli\0.10.1.yaml { gh --version }
--> Validating Manifest
Manifest validation succeeded.

--> Downloading dependencies

--> Starting Windows Sandbox, and:
    - Mounting the following directories:
      - C:\Users\felip\AppData\Local\Temp\SandboxTest as read-only
      - C:\Users\felip\Repos\winget-pkgs as read-and-write
    - Installing WinGet
    - Installing the Manifest .\manifests\GitHub\cli\0.10.1.yaml
    - Refreshing environment variables
    - Running the following script:

{
 gh --version
}

A GIF is worth a thousand words.

U6ho8iV7OL

To-do list

  • Download winget .Appx files on the fly, including the dependencies such as the VCLibs (and remove them from git, of course)

  • Validate Manifest file with winget validate before pushing to Sandbox test

  • Install winget in Sandbox

  • Run winget install -m with the provided manifest file in Sandbox

  • Add ability to run the custom script as the last step in Sandbox

  • Automatically refresh the environment variables after installing the Manifest

  • Close Windows Sandbox automatically if needed

  • Provide a way to specify which folder to mount, default is the current

Fixes microsoft/winget-cli#253, despite personally I would love to see this integrated into winget-cli itself, so at least users would not need to clone the repository before testing.

Microsoft Reviewers: Open in CodeFlow

@felipecrs felipecrs changed the title Create SandboxTest script [WIP] Create SandboxTest script May 22, 2020
@felipecrs
Copy link
Contributor Author

I need help with the winget validate, it always returns 0 as status code so I'm not sure how can I catch errors in it. Does anyone have any suggestions?

@felipecrs
Copy link
Contributor Author

I also need to know where I can get the VCLibs and VCLibsUWP .Appx files, which need to be installed as dependencies for the DesktopAppInstaller. Does anyone know?

I tried the .Appx provided by this package, but they are not up-to-date and does not meet the requirements for the DesktopAppInstaller.

@felipecrs felipecrs mentioned this pull request May 23, 2020
@chausner
Copy link
Contributor

chausner commented May 23, 2020

Very cool. Thanks for sharing this prototype script!

I need help with the winget validate, it always returns 0 as status code so I'm not sure how can I catch errors in it. Does anyone have any suggestions?

Good point. winget validate should indeed exit with a non-zero error code if validation fails (I created issue microsoft/winget-cli#312). For now, you could just parse the output and look for the string "Manifest validation succeeded.".

@chausner
Copy link
Contributor

I also need to know where I can get the VCLibs and VCLibsUWP .Appx files, which need to be installed as dependencies for the DesktopAppInstaller. Does anyone know?

I tried the .Appx provided by this package, but they are not up-to-date and does not meet the requirements for the DesktopAppInstaller.

People wanting to install Windows Terminal outside of the Store have run into the same issue. An up-to-date version of the dependencies is available in the Store: https://www.microsoft.com/store/productId/9NBLGGH4RV3K

Unfortunately, there does not seem to be a way to automate its installation. A manual workaround is described here.

@felipecrs
Copy link
Contributor Author

Good point. winget validate should indeed exit with a non-zero error code if validation fails (I created issue microsoft/winget-cli#312). For now, you could just parse the output and look for the string "Manifest validation succeeded.".

Good idea! I did it.

@felipecrs
Copy link
Contributor Author

felipecrs commented May 23, 2020

People wanting to install Windows Terminal outside of the Store have run into the same issue. An up-to-date version of the dependencies is available in the Store: https://www.microsoft.com/store/productId/9NBLGGH4RV3K

I used https://store.rg-adguard.net/ to download the .Appx from the Store. However, despite the links for the .Appx are directly from Microsoft, they expire, so we can't use them for downloading.

We can still upload these two .Appx somewhere, such as in GitHub releases, but I don't know if this is allowed or not.

Unfortunately, there does not seem to be a way to automate its installation. A manual workaround is described here.

I had read this before, but I didn't get what was the manual workaround.

It seems that the Windows Terminal installer now ships the dependencies builtin, and if that's true, this could solve the problem. But an open point is that someone said the installer just tells the Store to download the dependencies automatically, if that's the case this wouldn't help since the Windows Sandbox doesn't have access to the Store.

@chausner
Copy link
Contributor

I also need to know where I can get the VCLibs and VCLibsUWP .Appx files, which need to be installed as dependencies for the DesktopAppInstaller. Does anyone know?

Since the script requires winget to be installed, those VCLibs dependencies should also be guaranteed to be present on the host system. I tried building the appx files from C:\ProgramFiles\WindowsApps\Microsoft.VCLibs.. by simply zipping the folders but for some reason the resulting appx files become corrupt. Even taking a valid appx, unzipping and zipping it again, makes them corrupt. Still trying to figure out why.

@felipecrs
Copy link
Contributor Author

@chausner Right. But did you tried with the files in this PR? The binaries are in the branch until I solve that.

@chausner
Copy link
Contributor

@chausner Right. But did you tried with the files in this PR? The binaries are in the branch until I solve that.

Yes, the current solution works fine (apart from this minor issue https://github.com/felipecassiors/winget-pkgs/pull/1). I was just checking if there is an alternative way to obtain the dependencies.

chausner and others added 2 commits May 24, 2020 21:36
The dash character in -ExecutionPolicy was a special character which broke the command.
@felipecrs felipecrs changed the title [WIP] Create SandboxTest script Create SandboxTest script May 28, 2020
@felipecrs
Copy link
Contributor Author

@chausner Can you please take a look now? I removed the binaries from the branch, and now the script is able to download them if needed.

The only thing missing now is the URLs for the VCLibs dependencies (not a problem if we can upload them to somewhere).

@felipecrs
Copy link
Contributor Author

felipecrs commented May 28, 2020

This PR is ready for review. And it's already usable.

YutrU9BLac

@felipecrs
Copy link
Contributor Author

@Xeevis this is intended, we just rely on the status code returned by winget here. Providing a way to not fail on warnings would be a new CLI switch for winget itself, consider opening an Issue there if you think it's good.

@felipecrs
Copy link
Contributor Author

felipecrs commented Feb 5, 2021

I'll think about adding a switch to skip the validation, though. For now, you can do something like:

.\SandboxTest.ps1 { winget install -m 10.0.9.yaml }

As long as 10.0.9.yaml is in the current folder.

@denelon
Copy link
Contributor

denelon commented Apr 27, 2021

@felipecrs have you been able to confirm if this works for the current version of the client with the new manifest schema? I see this PR has been open for a long time. It might be time to consider merging this one :)

@felipecrs
Copy link
Contributor Author

I'm going to try soon, @denelon.

@ghost ghost removed the Needs: Attention label Apr 27, 2021
@felipecrs
Copy link
Contributor Author

@denelon

  1. Added -SkipManifestValidation (Create SandboxTest script #827 (comment))
  2. Fix TLS issues with Invoke-WebRequest and GitHub API.
  3. Fix checksum file name convention in the winget-cli GitHub releases
  4. Fix issues with PowerShell 7 in my tests
  5. Remove the unneeded VCLibs dependency which was using my own URL (yay!)
  6. Add compatibility with the new manifest schema (folders instead of files)

I think it's good to merge!

@denelon
Copy link
Contributor

denelon commented Apr 27, 2021

@felipecrs how did you remove the dependency?

I'll ask one of the engineers to give this a review :)

@felipecrs
Copy link
Contributor Author

how did you remove the dependency?

@denelon, well, it seems that the newest version of winget-cli does not depend on that specific appx anymore, I simply removed it, and it's working still.


Just provide the path to manifest as parameter:
```powershell
.\Tools\SandboxTest.ps1 <path-to-manifest>
Copy link
Contributor

Choose a reason for hiding this comment

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

I kept passing the manifest version file itself and validation kept failing, took me a while to realize this parameter expects the manifest version directory. I'd clarify that here and in the param help

Copy link
Contributor Author

@felipecrs felipecrs May 1, 2021

Choose a reason for hiding this comment

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

@palenshus Well, my goal was to support the two kinds of manifests to keep backwards compatibility (it was files previously). Since you said you're not from the winget-cli team, it may be better to ask them about it.

@denelon, any suggestion?

If yet we decide to keep accepting both files and folders, the help can be surely enhanced to clarify that.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a singleton manifest you can point to the file, but the directory option works for both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I was only trying it on multi-file schemas. I'd just make it take a directory in both cases for consistency. Winget will validate the directory and ensure that if the manifestType is singleton, then there's only the one file in there

if (-Not $SkipManifestValidation -And -Not [String]::IsNullOrWhiteSpace($Manifest)) {
Write-Host '--> Validating Manifest'

if (-Not (Test-Path -Path $Manifest)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should have -PathType Container now too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on the other conversation also.


Param(
[Parameter(Position = 0, HelpMessage = "The Manifest to install in the Sandbox.")]
[String] $Manifest,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be declared mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of the script is to be flexible. Some times you just want to open sandbox, have winget installed, the current folder mounted and mess with it freely, this allows:

> .\Tools\SandboxTest.ps1 # works
> .\Tools\SandboxTest.ps1 -Script { winget install git; Update-Environment; git --version } # also works, no manifest

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I didn't realize that was an intended purpose, makes sense


Get-ChildItem $tempFolder -Recurse -Exclude $dependencies.fileName | Remove-Item -Force -Recurse

if (-Not [String]::IsNullOrWhiteSpace($Manifest)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside from making this mandatory in the params, I'd check this once up top and fail out if it's whitespace, then you don't have to keep checking it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also depends on the other conversation, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving manifest optional is fine


# Create Bootstrap script

# See: https://stackoverflow.com/a/14382047/12156188
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this for a little more clarity. Maybe Update-EnvironmentVariables?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this implementation seems a little less fraught to me? https://stackoverflow.com/a/22670892/892770

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update-EnvironmentVariables

Well, it's really a long string, especially if you consider that it will be typed manually. But that's how PowerShell is, I agree with this change.

Also, this implementation seems a little less fraught to me? https://stackoverflow.com/a/22670892/892770

Definitely. I don't know why I picked the first one, but I'll try and if it works I'll switch.





Copy link
Contributor

Choose a reason for hiding this comment

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

Why so much whitespace?

Copy link
Contributor Author

@felipecrs felipecrs May 1, 2021

Choose a reason for hiding this comment

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

Because of this (the progress bar stays on the screen even after the completion):

U6ho8iV7OL

One option would be to hide the progress bar though, but I just wanted to make the script as transparent as possible. I even though about turning on the PowerShell debug feature to output each command being runned, but it turned out to be too noisy.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, keep it then no prob

@palenshus
Copy link
Contributor

Very cool, this is super impressive! Just got a few comments here and there

Copy link
Contributor Author

@felipecrs felipecrs left a comment

Choose a reason for hiding this comment

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

Thank you so much for the comments, I'll work on them, but a few I still require some inputs.

Copy link
Contributor

@palenshus palenshus left a comment

Choose a reason for hiding this comment

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

Looks good to me

@denelon denelon dismissed KevinLaMS’s stale review May 3, 2021 18:36

The library dependencies have been removed.

@denelon denelon merged commit 9619e1a into microsoft:master May 3, 2021
@felipecrs felipecrs deleted the sandbox-test branch May 4, 2021 03:30
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.

Test in Windows Sandbox