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

Deploy copy-local package satellites correctly #1224

Merged
merged 2 commits into from
May 19, 2017

Conversation

nguerrera
Copy link
Contributor

@nguerrera nguerrera commented May 17, 2017

Customer scenario

Targeting desktop and consuming any package containing satellite assemblies. The satellite assemblies were not deployed to the correct location (on build, publish was OK) and so they simply do not load as expected.

Bugs this fixes

#1006

Workarounds, if any

None.

Risk

Low. The change is scoped to the case that is failing.

Performance impact

Minimal. Some additional path computation is done, but only in the case that is currently broken.

Root cause analysis

Lack of test coverage for intersection of desktop-targeting and packages with satellite assemblies. Test gap is closed along with the change.

How was the bug found?

Customer reported and identified as blocking on several email threads with customers and partners.

@dsplaisted @dotnet/dotnet-cli


<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net46</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

In #1132, which was closed as a dupe of #1006 which this PR purports to fix, the repro occurs only when TargetFrameworks is defined in the project and TargetFramework is now. Are we sure it fixes #1132? Should we add another test to covers that scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the coverage.

The diff shown in #1132 also changes from netstandard1.4 to net46. Are you sure that you had <TargetFramework>net46</TargetsFramework> working and only <TargetFrameworks>net46[;...]</TargetFrameworks> failing? This very test repros the issue without using TargetFrameworks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I don't remember whether I noticed and checked the one case without conflating the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the coverage in 7ba7a66 to be safe. I can't see from code inspection how TargetFramework vs TargetFrameworks would make any difference so I suspect things were conflated.

@nguerrera
Copy link
Contributor Author

@MattGertz for approval

@nguerrera nguerrera merged commit 67fd3a1 into dotnet:master May 19, 2017
@nguerrera nguerrera deleted the fix-1006 branch May 19, 2017 17:15
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…117.2 (dotnet#1224)

- Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20067.2
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