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

Use a better error message when TargetFramework property has a semicolon in it #1274

Merged

Conversation

dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented May 31, 2017

Fixes #385

For checking for a TargetFramework value with a semicolon in it, this reuses the existing check that TargetFrameworkIdentifier or TargetFrameworkVersion couldn't be inferred, but uses a different error message:

The TargetFramework value 'netcoreapp2.0;net46' is not valid. To multi-target, use the 'TargetFrameworks' property instead.

Fixes #1015

Updates the error message in the case where there isn't a semicolon in TargetFramework to:

The TargetFramework value 'foo45' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly.

This PR also updates the _CheckForUnsupportedTargetFramework target to run before Restore, since a valid target framework is necessary in order to restore.

@dsplaisted
Copy link
Member Author

@dotnet-bot
test Windows_NT Release
test Windows_NT_FullFramework Debug

@dsplaisted dsplaisted requested review from nguerrera and livarcocc May 31, 2017 15:31
<Target Name="_CheckForUnsupportedTargetFramework" BeforeTargets="_CheckForInvalidConfigurationAndPlatform">
<NETSdkError Condition="'$(_UnsupportedTargetFrameworkError)' == 'true'"
<Target Name="_CheckForUnsupportedTargetFramework"
BeforeTargets="_CheckForInvalidConfigurationAndPlatform;Restore;CollectPackageReferences"
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, this won't run before restore unless it's the root project. We use a private target (_GenerateRestoreSpec or something, search for it) with @emgarten's approval elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

GenerateRestoreProjectSpec is the per project outer build target that restore executes. As mentioned restore is only called once and that might be in the root project or in the solution metaproj depending on how the restore was done.

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets#L423

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to go in inner build as it's validating targetframework singular. Do we need to intercept restore? Doesn't restore already have a good enough error for bad targetframework?

Copy link
Member Author

Choose a reason for hiding this comment

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

The message from restore is:

error : Invalid restore input. Invalid target framework 'unsupported'. Input files: C:\git\dotnet-sdk\bin\repro\WrongFramework\WrongFramework.csproj.

Note that it says that the target framework is "unsupported" for any invalid value of TargetFramework.

Also, in the case that you have multiple valid target frameworks separated by semicolons in the TargetFramework property, the restore operation appears to just work, restoring for both.


<NETSdkError Condition="$(TargetFramework.Contains(';'))"
ResourceName="TargetFrameworkWithSemicolon"
FormatArguments="$([MSBuild]::Escape('$(TargetFramework)'))" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see tests for both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added both tests

@dsplaisted
Copy link
Member Author

@MattGertz for approval

Customer scenario

Setting the TargetFramework in the project file. This PR improves the error messages that are generated.

Bugs this fixes:

#385 and #1015

Workarounds, if any

n/a - This change makes error messages more helpful

Risk

Low - This changes the text of an error, and adds a different error message in a special case

Performance impact

Low - a few extra conditional evaluations during build

Is this a regression from a previous update?

No

Root cause analysis:

n/a

How was the bug found?

dogfooding

@dsplaisted dsplaisted merged commit bbdf894 into dotnet:release/2.0.0 Jun 3, 2017
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…0200213.23 (dotnet#1274)

- Microsoft.AspNetCore.Analyzers - 5.0.0-preview.1.20113.23
- Microsoft.AspNetCore.Mvc.Analyzers - 5.0.0-preview.1.20113.23
- Microsoft.AspNetCore.Components.Analyzers - 5.0.0-preview.1.20113.23
- Microsoft.AspNetCore.Mvc.Api.Analyzers - 5.0.0-preview.1.20113.23
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.

6 participants