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.

Sign up for GitHub

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

Jump to bottom

Commonize Hybrid CRT support #1026

Merged
merged 20 commits into from
Aug 17, 2021
Merged

Commonize Hybrid CRT support #1026

merged 20 commits into from
Aug 17, 2021

Conversation

DrusTheAxe
Copy link
Member

@DrusTheAxe DrusTheAxe commented Jul 6, 2021 •
edited
Loading

Move hybrid CRT support out of individual project files and up to the project level as a whole

This is a build generalization of Use the Universal C Runtime #888. It changes no product binaries, just test binaries that weren't previously using the hybrid CRT technique.

Verified by comparing file sizes and import (LINK /DUMP /IMPORTS) of binaries' previously using hybrid CRT (before vs after). Also reviewed app binaries' (product+test) imports to verify no VS CRT imports.

NOTE: This PR intentionally doesn't change the installer as its build structure's a bit different and needs additional changes. That'll come in a following PR

@DrusTheAxe DrusTheAxe added the area-Infrastructure Build, test, source layout, package construction (TODO: move to Deployment, DeveloperTools) label Jul 6, 2021
@DrusTheAxe DrusTheAxe added this to the 1.0 (2021 Q4) milestone Jul 6, 2021
@DrusTheAxe DrusTheAxe self-assigned this Jul 6, 2021
@ghost ghost added the needs-triage label Jul 6, 2021
@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DrusTheAxe DrusTheAxe requested a review from alexlamtest July 6, 2021 06:14
@riverar
Copy link
Contributor

riverar commented Jul 6, 2021 •
edited
Loading

Wow, it looks like VC16 is now explicitly blocking the hybrid CRT scenario? 👀 Or were those MRT build failures always there?

@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DrusTheAxe
Copy link
Member Author

Wow, it looks like VC16 is now explicitly blocking the hybrid CRT scenario? 👀 Or were those MRT build failures always there?

That's a surprise. MRT always built before, all this did was move the definition to an earlier point in the build flow. And MRT failed but ProjectReunion.sln is built before MRT and no problems there? How peculiar. Spelunking...

@Scottj1s
Copy link
Member

Scottj1s commented Jul 17, 2021 •
edited
Loading

Wow, it looks like VC16 is now explicitly blocking the hybrid CRT scenario? 👀 Or were those MRT build failures always there?

That's a surprise. MRT always built before, all this did was move the definition to an earlier point in the build flow. And MRT failed but ProjectReunion.sln is built before MRT and no problems there? How peculiar. Spelunking...

dev\MRTCore\mrt\Microsoft.Windows.ApplicationModel.Resources\src\Microsoft.Windows.ApplicationModel.Resources.vcxproj is building for store deployment, which prohibits static CRT. Should be able to simply remove these:

    <AppContainerApplication>true</AppContainerApplication>    
    <ApplicationType>Windows Store</ApplicationType>
    <ApplicationTypeRevision>10.0</ApplicationTypeRevision>

@riverar
Copy link
Contributor

riverar commented Jul 17, 2021

@Scottj1s Good catch!

@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DrusTheAxe
Add DesktopCompatible to MrtCoreManagedTest for future reference - it…
386d3cc 
… doesn't hurt, but it doesn't help (at least, not this issue). Also added Win32Proj keyword to M.AM.Resources project. We now have 1 root issue
@DrusTheAxe
Removed MrtCoreManagedTest from the build due to a fundamental incomp…
6f4b23d 
…atibility (ut's a Universal Windows project referencing Microsoft.ApplicationModel.Resources (the WinRT dll) which isn't, and VS doesn't like crossing streams. Even worse, VS and msbuild produce different results!).
@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

huichen123
huichen123 requested changes Aug 14, 2021
View reviewed changes
Copy link
Contributor

@huichen123 huichen123 left a comment

Choose a reason for hiding this comment

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

Please do not disable the test.

EHO-makai
EHO-makai reviewed Aug 16, 2021
View reviewed changes
Copy link
Contributor

@EHO-makai EHO-makai left a comment

Choose a reason for hiding this comment

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

Did the build issues get resolved? What test is failing? I couldn't quite follow the details from the conversation.

Overall, it looks fine but it looks like there are some tail end issues blocking completion?

@DrusTheAxe
Copy link
Member Author

Did the build issues get resolved?

If you mean the test issue, no. Under discussion

What test is failing?

MrtCoreManagedTest

@DrusTheAxe
Merge branch 'main' into user/drustheaxe/ucrt
cd184cc
@DrusTheAxe
Revert "Removed MrtCoreManagedTest from the build due to a fundamenta…
a03f0ce 
…l incompatibility (ut's a Universal Windows project referencing Microsoft.ApplicationModel.Resources (the WinRT dll) which isn't, and VS doesn't like crossing streams. Even worse, VS and msbuild produce different results!)."

This reverts commit 6f4b23d.
@DrusTheAxe
Doing heinous things to workaround MrtCoreManagedTest is a Univesal W…
411ba67 
…indows project. Epic Failure. Because it's a Universal Windows project MSTest is creating $(OutDir)AppX subdir and copying files ***it knows about*** to the subdir. This brutal hack fails because File.Exists() for files in the parent directory fails, and that's because the last var pc = ... tries to access the AppX directory's parent ***and fails because Access Denied***. Previously doing the copy in the .csproj fails because the AppX directory doesn't necesarily exist before the test runs, and MD $(OutDir)AppX before copying files is pointless because MSTest is deleting all the files (if any) in the AppX directory when the test starts. So dead end upon dead end. Only way is to find a way to bench the Universal Windows .props logic to understand the files it doesn't want to allow through the ...Transitive... copy rules. Or just rewrite the project as a non-Universal Windows project. Or rewrite the test using TAEF instead of MSTest. Or... drink heavily. Hunting up Scott tomorrow hoping he has an idea
@DrusTheAxe
Hah! Thank you ScottJ! ReferenceCopyLocalPaths does the right thing, …
96902f8 
…copying the needed DLLs into $(OutDir) *and* $(OutDir)AppX and the tests now find the dll and pass
@DrusTheAxe
Remove debugging hackery
22bac61
@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DrusTheAxe
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DrusTheAxe DrusTheAxe requested a review from huichen123 August 17, 2021 19:45
huichen123
huichen123 approved these changes Aug 17, 2021
View reviewed changes
Copy link
Contributor

@huichen123 huichen123 left a comment

Choose a reason for hiding this comment

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

:shipit:

@DrusTheAxe DrusTheAxe merged commit af506d6 into main Aug 17, 2021
@DrusTheAxe DrusTheAxe deleted the user/drustheaxe/ucrt branch August 17, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huichen123 huichen123 huichen123 approved these changes

@BenJKuhn BenJKuhn Awaiting requested review from BenJKuhn

@Scottj1s Scottj1s Awaiting requested review from Scottj1s

@aeloros aeloros Awaiting requested review from aeloros

@rohanp-msft rohanp-msft Awaiting requested review from rohanp-msft

@kythant kythant Awaiting requested review from kythant

@alexlamtest alexlamtest Awaiting requested review from alexlamtest

@MikeHillberg MikeHillberg Awaiting requested review from MikeHillberg

@dhoehna dhoehna Awaiting requested review from dhoehna

@huzaifa-d huzaifa-d Awaiting requested review from huzaifa-d

@EHO-makai EHO-makai Awaiting requested review from EHO-makai

Assignees

@DrusTheAxe DrusTheAxe

Labels
area-Infrastructure Build, test, source layout, package construction (TODO: move to Deployment, DeveloperTools) needs-triage
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@DrusTheAxe @riverar @Scottj1s @huichen123 @EHO-makai