Fix LightCommand packages support based on failures in dotnet/runtime. #5552
Conversation
|
This is transitively blocking dotnet/runtime#36715 which we'd like to get in before the cut for Preview 6, so if possible, can this get reviewed quickly? |
|
@chcosta is OOF on paternity leave for a few weeks. Does anyone else but /runtime use this yet? |
|
I believe dotnet/windowsdesktop uses this as well. |
|
Their usage is significantly out of date at the moment it looks like. |
|
After talking with @jkoritzinsky offline, the only repo right now that would be impact is /runtime. Given that they're blocked, I'm approving this PR. Also, I think @jcagme is looking into this on the linked issue. If per chance something comes up, then we'd expect a subsequent PR. |
| @@ -334,11 +334,12 @@ | |||
| Cultures="en-us" | |||
| Out="$(OutInstallerFile)" | |||
| WixExtensions="@(WixExtensions)" | |||
| WixSrcFiles="@(WixSrcFile -> '$(WixObjDir)%(Filename).wixobj');@(DirectoryToHarvest -> '%(WixObjFile)')" | |||
| Condition="'$(EnableCreateLightCommandPackageDrop)' == 'true'"> | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@markwilkie @jcagme the test failures look to be flaky tests with XHarness on iOS for Helix. This PR doesn't touch an SDK that would change those tests' behavior. Can we merge this in or should I re-run the failing leg? |
|
@MattGal - thoughts? |
|
fyi @jonfortescue |
I think I've addressed the only known source of XHarness Android test flakiness via #5555, just now. If you see failures like that and you aren't touching the Helix SDK, they're ignorable though I'd of course like to know about them. This one's iOS. Go ahead and merge your PR; you definitely didn't cause the problem. I'll investigate and get an issue filed in the dotnet/xharness repo. |
|
Looks like this is the issue Premek was already fixing, so #5555 brought what we hope is the fix for this too (fingers crossed). Thanks for your patience while we find the little stabilizing bits of this new CLI. |
This PR fixes the issues found in the LightCommand support when trying to update the SharedFramework SDK in dotnet/runtime in dotnet/runtime#36889
cc: @chcosta and @jcagme for review.
Unblocks dotnet/runtime#36889