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

Add conditions for implicit framework references for .NET 3.5 and below #561

Merged
merged 2 commits into from
Jan 4, 2017

Conversation

dsplaisted
Copy link
Member

Fixes #532

There aren't any tests for this, as we run all our tests using the .NET CLI version of MSBuild, which doesn't support targeting .NET 3.5 and lower (as there are no reference assemblies for these frameworks).

@@ -77,18 +77,18 @@ Copyright (c) .NET Foundation. All rights reserved.
</PropertyGroup>

<ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETFramework'">
<!-- When doing greater than/less than comparisons between strings, MSBuild will try to parse the strings as Version objects and compare them as
Copy link
Member

Choose a reason for hiding this comment

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

Separate item groups with conditions would be clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's already a condition on the item group: Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETFramework'"

So splitting into separate item groups based on the target framework version would mean duplicating that condition and adding an additional clause for the version. I don't think that would end up being clearer.

Copy link
Member

Choose a reason for hiding this comment

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

Then manually group by similar conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, Done

@clairernovotny
Copy link
Member

Shouldn't a similar set of changes be included for the Xamarin platforms? They need/should have implicit references added as well to match, plus their platform lib Xamarin.iOS, MonoAndroid, etc?

@dsplaisted
Copy link
Member Author

@onovotny

Shouldn't a similar set of changes be included for the Xamarin platforms? They need/should have implicit references added as well to match, plus their platform lib Xamarin.iOS, MonoAndroid, etc?

I've added a comment on #491 to do this.

@dsplaisted dsplaisted merged commit 592fb91 into dotnet:master Jan 4, 2017
@dsplaisted dsplaisted deleted the target-net20-532 branch January 4, 2017 16:26
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 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.

6 participants