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 parallel builds by specifying the application type for WAP #7783

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Sep 29, 2020

The WAP packaging project is sensitive to including applications that it
thinks are UWPs. The changes we made to separate WindowsStoreApp and
WindowsAppContainer weren't comprehensive enough to convince WAP that we
were not still UWPs.

Because of that, it would run sub-builds of each of these projects (and
all their dependencies) with an additional GenerateAppxPackageOnBuild
property set. The existence of this property caused MSBuild to think the
projects needed to be built again.

The WAP packaging project in VS <= 16.7.3 produces a global property as
part of its normal operation that causes MSBuild to flag our projects as
out-of-date and requiring a rebuild. By forcing this property to match
the WAP values, we can get consistent builds.

That property is "GenerateAppxPackageOnBuild", and WAP sets it to
*false*. When we set that, of course, we don't get an MSIX out of our
build pipeline. Therefore, we have to break our build into two phases --
build, then package.

This is just like #3412, but a couple versions of VS in the future.
If you want to read a pareto-identical commit message, go read that one.
zadjii-msft
zadjii-msft previously approved these changes Sep 29, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

this is absolutely bonkers

miniksa
miniksa previously approved these changes Sep 29, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Nnnnn. Yes.

@DHowett
Copy link
Member Author

DHowett commented Sep 29, 2020

@msftbot merge this when it's done building

@ghost
Copy link

ghost commented Sep 29, 2020

Hello @DHowett!

I think you told me that you want to delay the approval for a certain amount of time, but I am not confident that I have understood you correctly.

Please try rephrasing your instruction to me.

@DHowett
Copy link
Member Author

DHowett commented Sep 29, 2020

@msftbot merge this when 5 minutes do

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 29, 2020
@ghost
Copy link

ghost commented Sep 29, 2020

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 29 Sep 2020 23:18:32 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 29, 2020
@DHowett
Copy link
Member Author

DHowett commented Sep 29, 2020

This failed in x86 with no explanation. Really loving it.

@DHowett DHowett dismissed stale reviews from zadjii-msft and miniksa September 30, 2020 00:20

Fundamental change in approach.

@DHowett
Copy link
Member Author

DHowett commented Sep 30, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett
Copy link
Member Author

DHowett commented Sep 30, 2020

/azp run

@DHowett DHowett changed the title Fix our parallel (and repeating) builds (again) Fix our parallel builds by specifying the application type for WAP Sep 30, 2020
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett DHowett changed the title Fix our parallel builds by specifying the application type for WAP Fix parallel builds by specifying the application type for WAP Sep 30, 2020
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 30, 2020
@ghost
Copy link

ghost commented Sep 30, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@@ -11,6 +11,7 @@
<OpenConsoleUniversalApp>false</OpenConsoleUniversalApp>
<ApplicationType>Windows Store</ApplicationType>
<NoOutputRedirection>true</NoOutputRedirection>
<TargetPlatformIdentifier>Windows</TargetPlatformIdentifier>
Copy link
Member

Choose a reason for hiding this comment

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

<!-- LOAD BEARING CODE: If we don't say Windows here, the project system might get confused and start building and not building APPXs because of the implicit decisions on UWP. -->

Copy link
Member Author

Choose a reason for hiding this comment

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

idk about that one -- this feels more correct (like, just that we shoulda specified it).. but i can see the argument

@DHowett DHowett merged commit da4ca86 into master Sep 30, 2020
@DHowett DHowett deleted the dev/duhowett/vs1673 branch September 30, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants