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

Fix Repair-WinGetPackageManager cmdlet by retrieving dependencies from GitHub assets #4923

Merged
merged 7 commits into from
Oct 31, 2024

Conversation

ryfu-msft
Copy link
Contributor

@ryfu-msft ryfu-msft commented Oct 30, 2024

This change address an issue with Repair-WinGetPackageManager not installing the correct version of VCLibs before installing winget. The reason this issue came up was because the aka.ms links for the VCLibs package are deprecated and now point to an out of date package.

The solution here is to include both a DesktopAppInstaller_Dependencies.json and DesktopAppInstaller_Dependencies.zip asset along with each release. Repair-WinGetPackageManager will look for those files and check what dependencies are required from the json file. If any of those dependencies are missing, it will download the zip, extract, and install the required binaries based on the system architecture.

Testing:
Verified with the fresh Windows Sandbox install and trying out the assets on an earlier prerelease.

Microsoft Reviewers: Open in CodeFlow

Related to:

@ryfu-msft ryfu-msft requested a review from a team as a code owner October 30, 2024 15:50
Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

This design requires that both the json and archive files be present to operate. The initial design I communicated did not. It wouldn't take much change here to realign.

Some of my comments already indicate how to verify with only the json present, although that particular scenario isn't very useful.

What we may find more useful would be functioning (albeit inefficiently) if the json file is missing or corrupted. That would require:

  1. Placing the dependencies in the archive in architecture directories
  2. Changing the format string to mirror this
  3. Implementing a code path that installs all packages found in all appropriate architectures when we failed to find or parse the json

I think it at least worth doing 1 and 2, as they require very little change. 3 can be done at some future point if we want, but without having done 1 or 2 now it wouldn't work for archives we make now.

@@ -122,7 +122,6 @@ private async Task RepairStateMachineAsync(string expectedVersion, bool allUsers

if (seenCategories.Contains(currentCategory))
{
this.Write(StreamType.Verbose, $"{currentCategory} encountered previously");
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this output?


foreach (var entry in missingDependencies)
{
string fullPath = System.IO.Path.Combine(extractedDirectory.FullDirectoryPath, DependenciesAssetName, entry);
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this directory ends up in the path, unless you are expecting the archive to have every dependency under this one directory. I also don't know why we need to increase the length of the path just to put this name in.

Copy link
Contributor Author

@ryfu-msft ryfu-msft Oct 30, 2024

Choose a reason for hiding this comment

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

I made a mistake with how I created the test zip. I will fix this.

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 removed this extra directory as its not needed anymore. Basically, I compressed the folder containing the arch folders. I had to instead select all the arch folders and compress from there.

private const string License = "License1.xml";

// Format of a dependency package such as 'x64\Microsoft.VCLibs.140.00.UWPDesktop_14.0.33728.0_x64.appx'
private const string ExtractedDependencyPath = "{0}\\{1}_{2}_{3}.appx";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private const string ExtractedDependencyPath = "{0}\\{1}_{2}_{3}.appx";
private const string ExtractedDependencyPath = "{0}\\{1}_{2}_{0}.appx";

nit: I guess this isn't really that important, it just seems to suggest that the directory architecture and the file name architecture don't need to match.

If taken, update the format calls to remove the second architecture.

@mdanish-kh
Copy link
Contributor

Does this address #4778 as well?

@JohnMcPMS
Copy link
Member

Does this address #4778 as well?

It addresses the Repair-WinGetPackageManager portion of it, but we are not trying to address the broader availability through this change.

@ryfu-msft ryfu-msft merged commit cdd4b08 into microsoft:master Oct 31, 2024
9 checks passed
@ryfu-msft ryfu-msft deleted the fixRepair2 branch October 31, 2024 16:31
ryfu-msft added a commit that referenced this pull request Oct 31, 2024
ryfu-msft added a commit that referenced this pull request Oct 31, 2024
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