Skip to content

Exe do not inclue app package #2395

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

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

wli3
Copy link

@wli3 wli3 commented Jul 12, 2018

…soft.NETCore.App

Fix #2204 (comment)

@wli3 wli3 changed the base branch from release/2.1.2xx to release/2.2.1xx July 12, 2018 20:52
@wli3 wli3 requested review from dsplaisted and a team July 12, 2018 20:52
}

[Fact]
public void Packing_a_netcoreapp_2_0_DotnetCliTool_app_does_not_include_the_implicit_dependency()
Copy link
Member

Choose a reason for hiding this comment

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

Remove the "not" from this test name, it's testing the opposite.

@dsplaisted
Copy link
Member

@Pilchie Do we need any sort of approval to check in to 2.2.1xx? Also FYI this is a breaking change, but we think the impact is low, there is a workaround, and it is worth doing because it fixes a lot of issues.

<PackageReference Update="Microsoft.NETCore.App"
Condition="('$(OutputType)' != 'Exe') And ('$(_TargetFrameworkVersionWithoutV)' != '') And ('$(_TargetFrameworkVersionWithoutV)' >= '2.0')"
Condition="('$(_TargetFrameworkVersionWithoutV)' != '') And ('$(_TargetFrameworkVersionWithoutV)' >= '2.0') And ('$(PackageType)' != 'DotnetCliTool')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want another escape hatch than setting PackageType? What else does that impact?

Copy link
Member

Choose a reason for hiding this comment

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

I would say we already have another escape hatch: put the following in your project:

<ItemGroup>
    <PackageReference Update="Microsoft.NETCore.App" PrivateAssets="None" />
</ItemGroup>

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I'm convinced. :)

@Pilchie
Copy link
Member

Pilchie commented Jul 12, 2018

I think 2.2.1xx is fair game for now (with signoff, etc)

@wli3
Copy link
Author

wli3 commented Jul 12, 2018

Test failed due to project tools test assert don't have PackageType property

I believe to be valid project tool, you need to set that package type. So I added it to the test asset

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

Successfully merging this pull request may close these issues.

4 participants