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.

Sign up for GitHub

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

Jump to bottom

Issue a warning on restore about obsolete DotNetCliToolReference #2064

Merged
merged 2 commits into from
Mar 22, 2018
Merged

Issue a warning on restore about obsolete DotNetCliToolReference #2064

merged 2 commits into from
Mar 22, 2018

Conversation

natemcmaster
Copy link
Contributor

Resolves #2014

I will make a corresponding change to dotnet/cli to generate Microsoft.NETCoreSdk.BundledCliTools.props with a list of obsolete package IDs to match https://github.com/dotnet/cli/blob/master/build/BundledDotnetTools.props.

VS:

image

Command line:

image

FYI @muratg @DamianEdwards @prafullbhosale @divega @bricelam @Eilon

nguerrera
nguerrera suggested changes Mar 19, 2018
View reviewed changes
Copy link
Contributor

@nguerrera nguerrera left a comment

Choose a reason for hiding this comment

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

See above

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ObsoleteReferences.targets Outdated
BeforeTargets="CollectPackageReferences"
Condition=" '$(SuppressObsoleteDotNetCliToolWarning)' != 'true' ">
<Warning Code="DOTNET1018"
Text="Using DotNetCliToolReference to reference '%(_ReferenceToObsoleteDotNetCliTool.Identity)' is obsolete and can be removed from this project. This tool is bundled by default in the .NET Core SDK."
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be localized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to. How?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have NETSdkError task. We briefly had NETSdkWarning but I believe it was just deleted due to being unused. Resurrect it from history and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added NETSdkWarning.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ObsoleteReferences.targets Outdated
<Target Name="_WarnAboutObsoleteDotNetCliToolReferences"
BeforeTargets="CollectPackageReferences"
Condition=" '$(SuppressObsoleteDotNetCliToolWarning)' != 'true' ">
<Warning Code="DOTNET1018"
Copy link
Contributor

Choose a reason for hiding this comment

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

How was this code chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the highest DOTNET code in https://github.com/dotnet/sdk/blob/master/src/Tasks/Common/Resources/Strings.resx and added one.
https://github.com/dotnet/sdk/blob/master/src/Tasks/Common/Resources/Strings.resx#L195

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 those are mostly left over from Nat defensively putting a bunch of messages from project.json era dotnet and nuget in strings.resx before v1 loc deadline just in case we needed them. We don't set Code elsewhere but probably should. I'd follow the current pattern here and we can file a bug to assign codes and use throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, opened this issue to follow up #2065

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
@@ -394,7 +394,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<RunArguments Condition="'$(RunArguments)' == ''">$(StartArguments)</RunArguments>
</PropertyGroup>
</When>

Copy link
Contributor

Choose a reason for hiding this comment

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

This file has tons of unrelated whitespace changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the noise. VS Code made me do it.

This was referenced Mar 19, 2018
Merged
Closed
nguerrera
nguerrera reviewed Mar 19, 2018
View reviewed changes
...Microsoft/Microsoft.NET.Build.Extensions/Microsoft.NET.Build.Extensions.NETFramework.targets
@@ -20,6 +20,7 @@ Copyright (c) .NET Foundation. All rights reserved.

<UsingTask TaskName="GetDependsOnNETStandard" AssemblyFile="$(MicrosoftNETBuildExtensionsTasksAssembly)" />
<UsingTask TaskName="NETBuildExtensionsError" AssemblyFile="$(MicrosoftNETBuildExtensionsTasksAssembly)" />
<UsingTask TaskName="NETBuildExtensionsWarning" AssemblyFile="$(MicrosoftNETBuildExtensionsTasksAssembly)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have any extensions warnings yet so this is unnecessary task load. We can wait for first use to add.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually load is clearly lazy extensions error is busted now due to build refactoring breaking ifdef, so this is ok.

@natemcmaster
Copy link
Contributor Author

By the way, if I want to get this into preview2, should I change the branch this PR targets?

@natemcmaster
Copy link
Contributor Author

Ok. I'm going to retarget this PR to release/2.1.3xx to make sure this gets in for preview2.

@natemcmaster natemcmaster changed the base branch from master to release/2.1.3xx March 19, 2018 21:26
@natemcmaster
Copy link
Contributor Author

Tests are currently failing on Microsoft.NET.Publish.Tests.GivenThatWeWantToStoreAProjectWithDependencies.compose_multifile, but this seems to be unrelated to my changes. Is this a known issue?

@natemcmaster
Copy link
Contributor Author

@dotnet-bot test Ubuntu14.04 Debug
@dotnet-bot test Ubuntu16.04 Release
@dotnet-bot test OSX10.12 Release
@dotnet-bot test Windows_NT Release

@natemcmaster
Copy link
Contributor Author

@dotnet-bot test Windows_NT Release
@dotnet-bot test OSX10.12 Release

@natemcmaster
Copy link
Contributor Author

I'm still getting some test failures on a few CI runs that appear unrelated to my change. Am I okay to merge anyways?

@livarcocc livarcocc added this to the 2.1.3xx milestone Mar 21, 2018
@livarcocc
Copy link
Contributor

One last attempt.

@dotnet-bot test Windows_NT Release
@dotnet-bot test OSX10.12 Release

@livarcocc
Copy link
Contributor

@johnbeisner is looking at flaky SDK tests for prod con. I think we are just hitting evidence of that in regular CI as well.

@natemcmaster
Copy link
Contributor Author

@dotnet-bot test OSX10.12 Release please

@natemcmaster
Copy link
Contributor Author

CI pr tests finally passed ✅ :D

@natemcmaster natemcmaster merged commit 2d87ce7 into dotnet:release/2.1.3xx Mar 22, 2018
@natemcmaster natemcmaster deleted the obsolete-cli-refs branch March 22, 2018 23:53
This was referenced Mar 22, 2018
Closed
Closed
@natemcmaster natemcmaster mentioned this pull request Apr 6, 2018
Closed
JL03-Yue pushed a commit that referenced this pull request Mar 19, 2024
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20240…
4d2f4c5 
…112.5 (#2064)

[main] Update dependencies from dotnet/arcade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@livarcocc livarcocc livarcocc approved these changes

@nguerrera nguerrera nguerrera approved these changes

@dsplaisted dsplaisted Awaiting requested review from dsplaisted

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.3xx
Development

Successfully merging this pull request may close these issues.
3 participants
@natemcmaster @livarcocc @nguerrera