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

[Mono] MSBuild Task housekeeping #54485

Merged
merged 4 commits into from
Jun 28, 2021

Conversation

MaximLipnin
Copy link
Contributor

@MaximLipnin MaximLipnin commented Jun 21, 2021

#53873

  • added TargetFrameworkForNETCoreTasks property similar to NetCoreAppToolCurrent one as NetCoreAppToolCurrent will likely bump more aggressively between new sdks.
  • renamed TargetFrameworkForNETFramework to TargetFrameworkForNETFrameworkTasks

…ppToolCurrent one as NetCoreAppToolCurrent will likely bump more aggressively between new sdks
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@MaximLipnin MaximLipnin marked this pull request as ready for review June 22, 2021 12:25
@@ -3,5 +3,6 @@

<PropertyGroup>
<TargetFrameworkForNETFramework>net472</TargetFrameworkForNETFramework>
<TargetFrameworkForNetCore>net6.0</TargetFrameworkForNetCore>
Copy link
Member

Choose a reason for hiding this comment

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

In general, what criteria will push us to update this value?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: TargetFrameworkForNETCore?

Copy link
Member

Choose a reason for hiding this comment

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

Any new features we may need. We may not need to bump all that often.

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

As long as everyone is ok w/ the property name, I'm good

@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<TargetFramework>$(TargetFrameworkForNetCore)</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended just for tasks? If so, then we should name it accordingly. It's not clear from the name, what it is for, or when to 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.

Perhaps, but it looks quite consistent in other places, e.g. in MonoAOTCompiler task <TargetFrameworks>$(TargetFrameworkForNetCore);$(TargetFrameworkForNETFramework)</TargetFrameworks>

Copy link
Member

Choose a reason for hiding this comment

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

It seems like the property is intended just to be used in src/tasks. Not sure if TargetFrameworkForNETFramework is also intended to be just used in src/tasks as well, but if it is perhaps we can rename them both to be something like TargetFrameworkForNETCoreTasks and TargetFrameworkForNETFrameworkTasks?

Copy link
Member

Choose a reason for hiding this comment

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

@radical if it is intended for just the tasks, do you think the above would be proper, or did you have something else in mind?

Copy link
Member

Choose a reason for hiding this comment

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

.. but if it is perhaps we can rename them both to be something like TargetFrameworkForNETCoreTasks and TargetFrameworkForNETFrameworkTasks

Sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick search showed that the TargetFrameworkForNETFramework property is used within src\task only so I've made the following change:
TargetFrameworkForNETFramework -> TargetFrameworkForNETFrameworkTasks
TargetFrameworkForNetCore -> TargetFrameworkForNETCoreTasks

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@MaximLipnin
Copy link
Contributor Author

The failures are not related

@MaximLipnin MaximLipnin merged commit daeee34 into dotnet:main Jun 28, 2021
@MaximLipnin MaximLipnin deleted the mdbuild_task_housekeeping branch June 28, 2021 15:20
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 29, 2021
…bugger2

* origin/main: (78 commits)
  Fix unreached during dump. (dotnet#54861)
  Fix lowering usage of an unset LSRA field. (dotnet#54731)
  Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (dotnet#54786)
  Add perf_slow yaml (dotnet#54853)
  Faster type load for scenarios made more common by generic math (dotnet#54588)
  Make sure we consider buffer length when marshalling back Unicode ByValTStr fields (dotnet#54695)
  Add YieldProcessor implementation for arm (dotnet#54829)
  Remove ActiveIssue for dotnet#50968 (dotnet#54831)
  Enable System.Text.Json tests for Wasm AOT (dotnet#54833)
  Remove ActiveIssue for dotnet#51723 (dotnet#54830)
  Fix load exception on generic covariant return type (dotnet#54790)
  Obsolete X509Certificate2.PrivateKey and PublicKey.Key. (dotnet#54562)
  First round of converting System.Drawing.Common to COMWrappers (dotnet#54636)
  Fix alloc-dealloc mismatches (dotnet#54701)
  Add one-shot ECB methods
  [Mono] MSBuild Task housekeeping (dotnet#54485)
  Move iOS/tvOS simulator AOT imports in the Mono workload (dotnet#54821)
  Remove unnecessary char[] allocation from Uri.GetRelativeSerializationString (dotnet#54799)
  Reduce overhead of Enumerable.Chunk (dotnet#54782)
  Fix EnumMemberRefs always returning NULL (dotnet#54805)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants