Skip to content

Conversation

@wli3
Copy link

@wli3 wli3 commented May 8, 2019

No description provided.

@wli3 wli3 changed the title [discussion] Pack cannot run WIP [discussion] Fix pack cannot run May 8, 2019
@wli3 wli3 changed the title WIP [discussion] Fix pack cannot run WIP [discussion] Fix packasktool cannot run May 8, 2019
</ItemGroup>

<PropertyGroup>
<UseAppHost Condition=" '$(SelfContained)' != 'true' and '$(PackAsTool)' == 'true' ">false</UseAppHost>
Copy link
Author

@wli3 wli3 May 8, 2019

Choose a reason for hiding this comment

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

This is tricky. I don't have a good solution:

This line need to be inside a target. Since PackAsTool is set in csproj.
And it need to be before Microsoft.NET.Sdk.targets (basically command target), so "RunCommand" property can use the correct value.

I try to keep PackAsTool logic in one place. But I cannot in this case. So far that's where I think is the best

@peterhuene @nguerrera

Copy link
Author

Choose a reason for hiding this comment

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

ping :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the second best place to put this would be where we decide the AppHost default:

In the same way that ideally these targets wouldn't know about PackAsTool, you can almost argue that PackAsTool shouldn't need to know about above. We either keep PackAsTool with PackAsTool or UseAppHost with UseAppHost. And only the latter actually works. ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd second that. I think we can work this logic into the place we default UseAppHost.

It needs to be inside a target. Since PackAsTool is set in csproj.
And it need to be before Microsoft.NET.Sdk.targets (basically command
target), so "RunCommand" property can use the correct value.
@wli3 wli3 force-pushed the pack-cannot-run branch from 1b0b655 to e4d50ec Compare May 9, 2019 04:53
@wli3 wli3 changed the title WIP [discussion] Fix packasktool cannot run Fix packasktool cannot run May 9, 2019
@wli3
Copy link
Author

wli3 commented May 9, 2019

Edited according to the discussion. this is good to review now

@wli3 wli3 merged commit 372e67d into dotnet:master May 10, 2019
@wli3 wli3 deleted the pack-cannot-run branch May 10, 2019 16:50
dsplaisted pushed a commit to dsplaisted/sdk that referenced this pull request Feb 19, 2020
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.

3 participants