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

Arm64 pending modules #17972

Merged
merged 11 commits into from
May 11, 2022
Merged

Conversation

yuyoyuppe
Copy link
Collaborator

Summary of the Pull Request

What is this about:
This PR contains contributions from @snickler as part of the patch to add ARM64 configuration to PowerToys. I've split the work across projects to multiple commits for easier reviewing and attributed commits to the author. Thanks a lot, Jeremy!
This PR doesn't add PowerToys.sln changes to include arm64 configuration - that'd be a separate PR.
What is included in the PR:

How does someone test / validate:

Quality Checklist

  • Linked issue: Support ARM platform #490
  • 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.

@ghost
Copy link

ghost commented Apr 29, 2022

CLA assistant check
All CLA requirements met.

@crutkas
Copy link
Member

crutkas commented May 3, 2022

🔥🔥🔥🔥

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.

Looking at the diff, some of these contain full file changes? why is this the case? some line ending / whitespace changes?
Could you check those to make sure what the correct push should be?

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

..etc.. We can make a big cleanup here in bunch of proj files

src/Update/PowerToys.Update.vcxproj Outdated Show resolved Hide resolved
</ImportGroup>
<PropertyGroup Label="UserMacros" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
</ImportGroup> <PropertyGroup Label="UserMacros" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line should be here between and <PropertyGroup

<PropertyGroup Condition="'$(Configuration)'=='Debug'">
<TargetName>PowerToys.ShortcutGuideModuleInterface</TargetName>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
<PropertyGroup Condition="'$(Configuration)'=='Release'">
<TargetName>PowerToys.ShortcutGuideModuleInterface</TargetName>
</PropertyGroup>
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for this - targetname unconditional

src/modules/keyboardmanager/dll/KeyboardManager.vcxproj Outdated Show resolved Hide resolved
src/modules/powerrename/dll/PowerRenameExt.vcxproj Outdated Show resolved Hide resolved
src/modules/powerrename/lib/PowerRenameLib.vcxproj Outdated Show resolved Hide resolved
@snickler
Copy link
Collaborator

snickler commented May 6, 2022

I will clean these up and get updates in. A lot of the updates I made for the other projects were done with simplification to the logic afterwards.

@snickler
Copy link
Collaborator

snickler commented May 6, 2022

@yuyoyuppe - Can you grant me contributor rights to your PowerToys repo? I have a local commit full of fixes and didn't realize that GitHub checked out the branch from YOUR repo :)

@yuyoyuppe
Copy link
Collaborator Author

@snickler invited :)

@yuyoyuppe
Copy link
Collaborator Author

Looks like all of the comments by @stefansjfw are resolved.

@jaimecbernardo, looking at .gitattributes, we use CRLF for .cs, no other preferences are set. The overwhelming majority of sources and project files are LF though (git ls-files --eol | grep w/lf). The diffs you mentioned are CLRF->LF, so I guess it's ok.

Maybe we should also fill our .gitattributes to avoid the confusion in the future.

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.

LGTM! Great work! Did you try to build an x64 installer from this and test to verify it works? (Testing changed PowerToys as well)

@yuyoyuppe
Copy link
Collaborator Author

I've rebased on the latest main to include 3da341f and tested all modules - they appear to work using the local x64 installer.

@jaimecbernardo
Copy link
Collaborator

Thank you. I'll squash and merge once the tests pass.

@jaimecbernardo jaimecbernardo merged commit 6a2d9e4 into microsoft:main May 11, 2022
@yuyoyuppe yuyoyuppe deleted the arm64_pending_modules branch May 11, 2022 10:35
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.

5 participants