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

[Mono] MSBuild Task housekeeping #54485

Merged
merged 4 commits into from
Jun 28, 2021
Merged

[Mono] MSBuild Task housekeeping #54485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66890ea
Use a separate TargetFrameworkForNetCore property similar to NetCoreA…
MaximLipnin Jun 21, 2021
cb5bdfe
Merge branch 'main' into mdbuild_task_housekeeping
MaximLipnin Jun 24, 2021
6fddc09
Update the property names
MaximLipnin Jun 28, 2021
0dc92c3
Merge branch 'main' into mdbuild_task_housekeeping
MaximLipnin Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/tasks/AndroidAppBuilder/AndroidAppBuilder.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

<OutputType>Library</OutputType>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<EnableDefaultCompileItems>false</EnableDefaultCompileItems>
Expand Down
2 changes: 1 addition & 1 deletion src/tasks/AotCompilerTask/MonoAOTCompiler.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent);$(TargetFrameworkForNETFramework)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworkForNetCore);$(TargetFrameworkForNETFramework)</TargetFrameworks>
<OutputType>Library</OutputType>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<EnableDefaultCompileItems>false</EnableDefaultCompileItems>
Expand Down
2 changes: 1 addition & 1 deletion src/tasks/AppleAppBuilder/AppleAppBuilder.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworkForNetCore)</TargetFrameworks>
<OutputType>Library</OutputType>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<EnableDefaultCompileItems>false</EnableDefaultCompileItems>
Expand Down
2 changes: 1 addition & 1 deletion src/tasks/Crossgen2Tasks/Crossgen2Tasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworkForNetCore)</TargetFrameworks>
<OutputType>Library</OutputType>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<NoWarn>$(NoWarn),CA1050</NoWarn>
Expand Down
1 change: 1 addition & 0 deletions src/tasks/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

</PropertyGroup>
</Project>
2 changes: 1 addition & 1 deletion src/tasks/RuntimeConfigParser/RuntimeConfigParser.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent);$(TargetFrameworkForNETFramework)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworkForNetCore);$(TargetFrameworkForNETFramework)</TargetFrameworks>
<OutputType>Library</OutputType>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<EnableDefaultCompileItems>false</EnableDefaultCompileItems>
Expand Down
10 changes: 5 additions & 5 deletions src/tasks/WasmAppBuilder/WasmAppBuilder.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent);$(TargetFrameworkForNETFramework)</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworkForNetCore);$(TargetFrameworkForNETFramework)</TargetFrameworks>
<Nullable>enable</Nullable>
<NoWarn>$(NoWarn),CA1050</NoWarn>
<!-- Ignore nullable warnings on net4* -->
Expand Down Expand Up @@ -37,10 +37,10 @@
<_PublishFramework Include="$(TargetFrameworks)" />

<!-- non-net4* -->
<FilesToPackage Include="$(OutputPath)$(NetCoreAppToolCurrent)\$(MSBuildProjectName)*"
TargetPath="tasks\$(NetCoreAppToolCurrent)" />
<FilesToPackage Include="$(OutputPath)$(NetCoreAppToolCurrent)\publish\System.Reflection.MetadataLoadContext.dll"
TargetPath="tasks\$(NetCoreAppToolCurrent)" />
<FilesToPackage Include="$(OutputPath)$(TargetFrameworkForNetCore)\$(MSBuildProjectName)*"
TargetPath="tasks\$(TargetFrameworkForNetCore)" />
<FilesToPackage Include="$(OutputPath)$(TargetFrameworkForNetCore)\publish\System.Reflection.MetadataLoadContext.dll"
TargetPath="tasks\$(TargetFrameworkForNetCore)" />

<!-- for net472 we need all the dependent assemblies too, so copy from the publish folder -->
<FilesToPackage Include="$(OutputPath)$(TargetFrameworkForNETFramework)\publish\*"
Expand Down
2 changes: 1 addition & 1 deletion src/tasks/WasmBuildTasks/WasmBuildTasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<TargetFramework>$(TargetFrameworkForNetCore)</TargetFramework>
<OutputType>Library</OutputType>
<Nullable>enable</Nullable>
<NoWarn>$(NoWarn),CA1050</NoWarn>
Expand Down
2 changes: 1 addition & 1 deletion src/tasks/WorkloadBuildTasks/WorkloadBuildTasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<TargetFramework>$(TargetFrameworkForNetCore)</TargetFramework>
<OutputType>Library</OutputType>
<Nullable>enable</Nullable>
<NoWarn>$(NoWarn),CA1050</NoWarn>
Expand Down
4 changes: 2 additions & 2 deletions src/tasks/installer.tasks/installer.tasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent);net461</TargetFrameworks>
<TargetFrameworks>$(TargetFrameworkForNetCore);net461</TargetFrameworks>
<IsShipping>false</IsShipping>
<RunAnalyzers>false</RunAnalyzers>
</PropertyGroup>
Expand All @@ -10,7 +10,7 @@
<PackageReference Include="NuGet.ProjectModel" Version="$(RefOnlyNugetProjectModelVersion)" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppToolCurrent)'">
<ItemGroup Condition="'$(TargetFramework)' == '$(TargetFrameworkForNetCore)'">
<PackageReference Include="Microsoft.Build" Version="$(RefOnlyMicrosoftBuildVersion)" />
<PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(RefOnlyMicrosoftBuildTasksCoreVersion)" />
</ItemGroup>
Expand Down