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

Move to WinAppSDK-1.1.0 #18603

Merged
merged 4 commits into from
Jun 4, 2022
Merged

Move to WinAppSDK-1.1.0 #18603

merged 4 commits into from
Jun 4, 2022

Conversation

stefansjfw
Copy link
Collaborator

@stefansjfw stefansjfw commented Jun 3, 2022

Summary of the Pull Request

note: Removed PowerRename method was breaking the build because of the unresolved reference. Method is not needed, therefore deleted.

Someone please test ARM64 installer.
What is this about:

What is included in the PR:

How does someone test / validate:

Quality Checklist

  • Linked issue: Solves some of the issues in Unable to load PowerToys settings app version 0.58.0 #18015
  • Communication: I've discussed this with core contributors in the issue.
  • Tests: Added/updated and all pass
  • Installer: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Docs: Added/ updated
  • Binaries: Any new files are added to WXS / YML

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@stefansjfw stefansjfw force-pushed the stefan/winappsdk1_1 branch from 7b47f98 to cd269c0 Compare June 3, 2022 23:42
@htcfreek
Copy link
Collaborator

htcfreek commented Jun 4, 2022

Does this fix some of the settings bugs?

@Jay-o-Way Jay-o-Way mentioned this pull request Jun 4, 2022
@Jay-o-Way
Copy link
Collaborator

Is this able to hop on .59 or has that train already left the station?

@jaimecbernardo
Copy link
Collaborator

Does this fix some of the settings bugs?

It will fix the "unable to run elevated" issue so many people are having.

@jaimecbernardo
Copy link
Collaborator

Is this able to hop on .59 or has that train already left the station?

Yes, this is what we were waiting for to include in .59 .

@@ -180,16 +179,6 @@ IFACEMETHODIMP CPowerRenameItem::GetId(_Out_ int* id)
return S_OK;
}

Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? Wouldn’t expect it in a rev for 1.1

Copy link
Collaborator

Choose a reason for hiding this comment

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

From the PR description. Looks like it isn't used and breaks the build.

Removed PowerRename method was breaking the build because of the unresolved reference. Method is not needed, therefore deleted.

@@ -73,3 +73,8 @@
[assembly: SuppressMessage("StyleCop.CSharp.MaintainabilityRules", "SA1402:File may only contain a single type", Justification = "Not part of the project. WindowsAppSDK file.", Scope = "type", Target = "~T:Microsoft.WindowsAppSDK.Runtime.Packages.Singleton")]
[assembly: SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1516:Elements should be separated by blank line", Justification = "Not part of the project. WindowsAppSDK file.", Scope = "type", Target = "~T:Microsoft.WindowsAppSDK.Runtime.Packages.Singleton")]
[assembly: SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1516:Elements should be separated by blank line", Justification = "Not part of the project. WindowsAppSDK file.", Scope = "type", Target = "~T:Microsoft.WindowsAppSDK.Runtime.Packages.Main")]
[assembly: SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1513:Closing brace should be followed by blank line", Justification = "Not part of the project. WindowsAppSDK file.", Scope = "namespaceanddescendants", Target = "Microsoft.WindowsAppSDK.Runtime.Packages")]
Copy link
Member

Choose a reason for hiding this comment

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

These are pretty important rules. Can we maybe target the offending file or method?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn’t block the release on this but would be nice to adjust

Copy link
Collaborator

Choose a reason for hiding this comment

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

The target part means these exceptions will be applied to the offending package. They'll still trigger in our code.
Regarding the copyright one, I think that could only be done locally in the file for it to be specific ... It's code from nuget, though. It's possible to make that one only apply to the Settings project, so I guess that's what we'll do.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Tested and it looks good. No longer has the run elevation issue with 1.1.
I've added a commit to revert the change where we were showing a warning instead of starting Settings under those conditions.
Great work.

@davidegiacometti
Copy link
Collaborator

Don't want to bother but both Settings and PowerRename are not starting after a clean build.
The error is 0xc0000005 access violation.
Could be my machine but never had any issue with SDK 1.0.3.

@jaimecbernardo
Copy link
Collaborator

Don't want to bother but both Settings and PowerRename are not starting after a clean build. The error is 0xc0000005 access violation. Could be my machine but never had any issue with SDK 1.0.3.

Wow. That's pretty weird. Any other info there? Like a call stack from VS?

@davidegiacometti
Copy link
Collaborator

image

Looks like the error is on Microsoft.WindowsAppRuntime.Bootstrap.Net.dll side.
Can I help somehow?

System.AccessViolationException
  HResult=0x80004003
  Message=Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
  Source=<Cannot evaluate the exception source>
  StackTrace:
<Cannot evaluate the exception stack trace>

@jaimecbernardo
Copy link
Collaborator

@davidegiacometti You've installed winappsdk 1.1, right? Any errors in the command line when installing it?
https://aka.ms/windowsappsdk/1.1/1.1.0/windowsappruntimeinstall-1.1.0-x64.exe

Did you restart after?

This is pretty weird.
Do you have event viewer events? I'm thinking creating an issue like this might help, which is a similar error but the exception code is different: microsoft/WindowsAppSDK#2573

Does the WinUI 3 Gallery from the store also crash in a similar fashion?

@jaimecbernardo
Copy link
Collaborator

@davidegiacometti , this issue microsoft/WindowsAppSDK#2492 seems to indicate somethings might need to be cleaned up. Can you try to really clean the repo with git clean -ffxd and trying to build again?

@davidegiacometti
Copy link
Collaborator

I agree that's really strange.

I have installed 1.1 runtime and both VS extensions (C# and C++).
Now I have:

  • Rebooted again
  • git clean -ffxd
  • Rebuild
  • Same error

I am also experiencing the error with WinUI 3 Gallery.

@davidegiacometti
Copy link
Collaborator

I have been able to fix it:

  • Removed all App SDK installations: 0.8, 1.0.3, several 1.1 previews.
  • Rebooted
  • Reinstalled 1.0.3 and 1.1
  • Everything works

I think the build was failing becouse of 1.1 previews installed.

@jaimecbernardo
Copy link
Collaborator

Thanks for the heads-up @davidegiacometti . This is something I definitely want to repro locally to open an issue upstream.

@jaimecbernardo jaimecbernardo merged commit 97be8db into main Jun 4, 2022
jaimecbernardo pushed a commit that referenced this pull request Jun 4, 2022
* Move to WinAppSDK-1.1.0

* expect.txt

* Revert "[Settings]Don't launch if explorer is running elevated"

This reverts commit c485da2.

* Make copyright header analyze suppression module scope
@davidegiacometti
Copy link
Collaborator

Global suppressions can be reverted after microsoft/WindowsAppSDK#2569

@jaimecbernardo jaimecbernardo deleted the stefan/winappsdk1_1 branch June 14, 2022 23:32
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.

6 participants