Decide a course of actions for missing TargetFrameworks

[andrew-boyarshin]

Some projects (like ConfuserEx) specify only conditional TargetFrameworkVersions:

<TargetFrameworkVersion Condition=" !$(DefineConstants.Contains('NET45')) ">v4.0</TargetFrameworkVersion>
<TargetFrameworkVersion Condition=" $(DefineConstants.Contains('NET45')) ">v4.5</TargetFrameworkVersion>

It works in legacy project system, but fails sanity check in CPS, as TargetFrameworkIdentifier doesn't get defined by Microsoft.NET.TargetFrameworkInference.targets since _ShortFrameworkIdentifier doesn't get defined by same targets file since TargetFramework is missing (wow such depth of recursion).

Suggestion: If TargetFrameworks is empty, force-add to project file after all PropertyGroups:

<PropertyGroup Condition="'$(TargetRuntime)' == 'Managed'">
    <TargetFramework>net40</TargetFramework>
</PropertyGroup>

[hvanbakel]

Why don't we just block this, just don't convert and have them manually make clear what they're trying to do? So if we can't find a target framework, just error out.

Isn't this just an attempt at multi-targeting which is finally somewhat supported in the new project setup?

[andrew-boyarshin]

Well, that's strange. There is no point in preventing the user to convert if they are aware of net40 default in MSBuild. It is more like inconsistency in CPS MSBuild — some parts handle missing TargetFramework well, some do not. But even that doesn't prevent MSBuild from working — the problem is in MSBuild.Sdks.Extras, that is relying on the presence of one CPS-internal property. So instead of preventing the user we should just fix bugs in new Sdks for him.

[hvanbakel]

Are you saying that if you don't specify the targetframework, the new project style will just assume net40???? That's just weird...

On Wed, Aug 22, 2018, 19:44 Andrew Boyarshin notifications@github.com wrote:

Well, that's strange. There is no point in preventing the user to convert if they are aware of net40 default in MSBuild. It is more like inconsistency in CPS MSBuild — some parts handle missing TargetFramework well, some do not. But even that doesn't prevent MSBuild from working — the problem is in MSBuild.Sdks.Extras, that is relying on the presence of one CPS-internal property. So instead of preventing the user we should just fix bugs in new Sdks for him.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/hvanbakel/CsprojToVs2017/issues/171#issuecomment-415264378, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8FPAx2AZCcpyTU7TFNLniMhxIXhrPWks5uThb7gaJpZM4WH40k .

[andrew-boyarshin]

Legacy will. CPS will assume _, v0.0, but if we set TargetFrameworkIdentifier to .NETFramework, everything (even restore) will work fine.

[hvanbakel]

Ok, can you elaborate on the condition there? Why the '$(TargetRuntime)' == 'Managed'?

Also we could just set that in the main property group with the condition on the TargetFramework node, right?

[andrew-boyarshin]

@hvanbakel I'm still looking for the optimal solution.

Also we could just set that in the main property group with the condition on the TargetFramework node, right?

I'm afraid we can't (if you mean sth like Condition="'$(TargetFramework)' == ''"). MSBuild resolves conditions in such a way (at group evaluation moment), so that there can be no dependencies from conditions to property values within one PropertyGroup, only dependencies between PropertyGroups are allowed.

[hvanbakel]

I think this makes sense to me now. What about the '$(TargetRuntime)' == 'Managed'? Can you explain why that's the condition you want to put on the property group

[andrew-boyarshin]

@hvanbakel

I'm still looking for the optimal solution.

In fact, the condition is wrong (TargetRuntime is not set at the moment of the evaluation when TargetFramework is not set).