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 incremental build issue where Microsoft.WindowsAppRuntime.Bootstrap.dll is always written to output #4282

Closed
wants to merge 1 commit into from

Conversation

jevansaks
Copy link
Member

PostBuildEvent is unconditionally run and xcopy always does the copy, making downstream projects think that they need to rebuild too because their references rebuild.

Switch to using a Target that leverages the built-in Copy task that already does the timestamp checks for us.

</ItemDefinitionGroup>

<Target Name="CopyMicrosoftWindowsAppRuntimeBootstrapdllToOutDir" Condition="'$(AppxPackage)' != 'true'" AfterTargets="Build">
Copy link
Member

Choose a reason for hiding this comment

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

Why is this conditioned on "not an appx package"? I guess because generally appx uses the FWP model of binding and not bootstrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I just maintained the condition from where it was previously. Probably a question for whoever added this -- @DrusTheAxe added in #827.

I also see that Howard tried to make a change to Copy in #2734 and put it back on #2758. Hopefully someone more knowledgeable about these can help verify this change doesn't re-break any of those scenarios. I suspect the change in #2734 didn't work because it wasn't invoking the task in a Target.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I'm generally supportive of the change. If you wanted a slam-dunk no-questions-asked change, just add the /d parameter to xcopy, which only copies if the source is newer than the target.

Copy link
Member

Choose a reason for hiding this comment

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

Why is this conditioned on "not an appx package"?
Because Bootstrapper is only needed when WinAppSDK MSIX packages via WinAppSDK DynamicDependencies, AND WinAppSDK DynDep only supports unpackaged apps.

But isn't this necessary-but-insufficient -- the condition should be "I an not a packaged app AND i am not using WinAppSDKSelfContained"?

Condition="('$(AppxPackage)' != 'true') and ('$(WindowsAppSDKSelfContained)' != 'true')"

?

</ItemDefinitionGroup>

<Target Name="CopyMicrosoftWindowsAppRuntimeBootstrapdllToOutDir" Condition="'$(AppxPackage)' != 'true'" AfterTargets="Build">
Copy link
Member

Choose a reason for hiding this comment

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

OK. I'm generally supportive of the change. If you wanted a slam-dunk no-questions-asked change, just add the /d parameter to xcopy, which only copies if the source is newer than the target.

</ItemDefinitionGroup>

<Target Name="CopyMicrosoftWindowsAppRuntimeBootstrapdllToOutDir" Condition="'$(AppxPackage)' != 'true'" AfterTargets="Build">
Copy link
Member

Choose a reason for hiding this comment

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

'$(AppxPackage)' != 'true'

Is that the right test or should it be '$(WindowsPackageType)'=='None' ?

@Scottj1s @bpulliam @evelynwu-msft what's the latest practices for "My VS project is not making an MSIX package"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This condition came from the ItemGroup above.

@jevansaks
Copy link
Member Author

/azp run

Copy link

No pipelines are associated with this pull request.

@jevansaks jevansaks enabled auto-merge (squash) March 27, 2024 20:42
@DrusTheAxe
Copy link
Member

/azp run

Copy link

No pipelines are associated with this pull request.

@kythant
Copy link
Contributor

kythant commented Mar 27, 2024

/azp run TransportPackage-Foundation-PR

Copy link

No pipelines are associated with this pull request.

@jonwis
Copy link
Member

jonwis commented Mar 28, 2024

@kythant or @bpulliam or @evelynwu-msft - what's going on with the build pipelines here?

@kythant
Copy link
Contributor

kythant commented Mar 28, 2024

@kythant or @bpulliam or @evelynwu-msft - what's going on with the build pipelines here?

The triggers won't work because the PR is coming from a forked copy of the repository. The branch cannot be seen from this pipeline https://microsoft.visualstudio.com/DefaultCollection/ProjectReunion/_build?definitionId=81063

@jonwis
Copy link
Member

jonwis commented Mar 28, 2024

@kythant or @bpulliam or @evelynwu-msft - what's going on with the build pipelines here?

The triggers won't work because the PR is coming from a forked copy of the repository. The branch cannot be seen from this pipeline https://microsoft.visualstudio.com/DefaultCollection/ProjectReunion/_build?definitionId=81063

So what should @jevansaks do? How do we get this built and validated?

@jevansaks
Copy link
Member Author

@kythant @bpulliam @evelynwu-msft @jonwis I strongly recommend you enable the settings on the ADO project and pipeline to enable building from forks to enable contributions. I sent email with the links you should follow.

@bpulliam
Copy link
Collaborator

bpulliam commented Mar 28, 2024 via email

@jevansaks
Copy link
Member Author

/azp run

Copy link

No pipelines are associated with this pull request.

@jevansaks
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jevansaks
Copy link
Member Author

@bpulliam Thanks, we also needed to change the pipeline triggers. But I seemed to have permissions for that.

@jevansaks
Copy link
Member Author

Builds from forks are enabled but without secrets they can't do anything. Abandoning for now until the team spends time to make forks work. Re-opened as #4296

@jevansaks jevansaks closed this Mar 28, 2024
auto-merge was automatically disabled March 28, 2024 16:06

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants