Skip to content

Fix LightCommand packages support based on failures in dotnet/runtime. #5552

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

Merged
merged 1 commit into from
May 27, 2020
Merged

Fix LightCommand packages support based on failures in dotnet/runtime. #5552

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk/targets/windows/wix.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,11 +334,12 @@
Cultures="en-us"
Out="$(OutInstallerFile)"
WixExtensions="@(WixExtensions)"
WixSrcFiles="@(WixSrcFile -> '$(WixObjDir)%(Filename).wixobj');@(DirectoryToHarvest -> '%(WixObjFile)')"
Condition="'$(EnableCreateLightCommandPackageDrop)' == 'true'">
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I haven't dug in to this, but why would the condition be problematic. At the moment, this is so early days for this feature and we don't want it on by default (imo)

Copy link
Member Author

Choose a reason for hiding this comment

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

The condition wasn't applied to the rest of the pipeline of files created/modified by the new feature, so if the feature was turned off (which it was by default) then later steps failed because files were missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

By only applying the condition here and none of the following steps, it effectively became required to opt-in to using this feature when uptaking a new version of the SharedFramework SDK.

This likely didn't ping during local tests since there were likely leftover files from testing the opt-in path when testing the opt-out path.

Copy link
Member

Choose a reason for hiding this comment

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

My preference would have been to just add the condition to the zip command below. I don't know of any other possible impact. It's not a big deal, it just means the feature is even further along than I intended (ie, on by default). Sorry about the break, glad this is resolved.

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 didn't know all of the places that I would have to add the condition. If it becomes a problem, we can always add the condition back in all the correct places and make it opt-in/opt-out instead of always-on.

WixSrcFiles="@(WixSrcFile -> '$(WixObjDir)%(Filename).wixobj');@(DirectoryToHarvest -> '%(WixObjFile)')">
<Output TaskParameter="LightCommandPackageNameOutput" PropertyName="_LightCommandPackageNameOutput" />
</CreateLightCommandPackageDrop>

<MakeDir Directories="$(LightCommandPackagesDir)" />

<ZipDirectory
DestinationFile="$(LightCommandPackagesDir)/LightCommandPackage-$(_LightCommandPackageNameOutput).zip"
Overwrite="true"
Expand Down