Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Apr 13, 2023

Removes the net472 target framework from both the project files and CI.

Also removes some accumulated hacks for supporting Mono/.NET Framework that are no longer needed.

XA test companion PR: dotnet/android#8041.

@jpobst jpobst force-pushed the no-net472 branch 11 times, most recently from ca978cc to 4c22a8b Compare April 14, 2023 03:50
@jpobst jpobst force-pushed the no-net472 branch 3 times, most recently from 6a0430c to 20c12f1 Compare May 11, 2023 19:26
jonpryor pushed a commit to dotnet/android that referenced this pull request May 15, 2023
Context: dotnet/java-interop#1099

Remove the `monoandroid10` target framework from `Mono.Android.csproj`
and `Mono.Android.Export.csproj`.

This enables removing `net472` from Java.Interop:
dotnet/java-interop#1099
@jpobst jpobst marked this pull request as ready for review May 16, 2023 00:43
@jpobst jpobst requested review from jonpryor and pjcollins May 16, 2023 00:44
<PackageVersion>1.0.0.0</PackageVersion>
<!-- These assemblies are used by Xamarin.Android.Build.Tasks which must be netstandard2.0
in order to be loaded into Visual Studio -->
<MSBuildRequiredTargetFramework>netstandard2.0</MSBuildRequiredTargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any docs to what this property does? I can't find any.

<ToolOutputFullPath>$(MSBuildThisFileDirectory)bin\$(Configuration)-$(TargetFramework.ToLowerInvariant())\</ToolOutputFullPath>
<TestOutputFullPath>$(MSBuildThisFileDirectory)bin\Test$(Configuration)-$(TargetFramework.ToLowerInvariant())\</TestOutputFullPath>
<PropertyGroup>
<IntermediateOutputPath>$(BaseIntermediateOutputPath)\$(Configuration)-$(DotNetTargetFramework.ToLowerInvariant())\</IntermediateOutputPath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we "follow convention" and instead use $(BaseIntermediateOutputPath)\$(Configuration)\$(DotNetTargetFramework.ToLowerInvariant())\? Since e.g. bin/Debug/class-parse.exe won't be built anymore, the only assemblies in bin/Debug will be those for .NET Standard, and there are fewer of those running around…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have an opinion here. I went back and forth on this and eventually chose to follow the convention we use in xamarin-android. I guess it's nice if you change branches between something that is net7.0 or net8.0 they won't clobber each other, but that's not much of a reason.


<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFramework>$(MSBuildRequiredTargetFramework)</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is where $(MSBuildRequiredTargetFramework) comes in? So it's not a "standard MSBuild-ism", but is instead something we use?

I think we should use a JI* prefix so that I don't get confused. :-) Perhaps $(JITargetFrameworkForMSBuildTasks)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's get @pjcollins opinion. It's modeled after the $(DotNetTargetFramework) that we use everywhere else.

We could also just hardcode netstandard2.0 back in the project files. I changed it because every time I see netstandard2.0 I want to upgrade it and I forget we can't because it's used by our MSBuild tasks. But I guess it will never change, so we could just leave it hardcoded in the project files.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion either way, I think netstandard2.0 is fine everywhere, or perhaps a rename to something like $(BuildTaskTargetFramework) or $(JIBuildTaskTargetFramework) would be 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.

Removed in favor of just using netstandard2.0.

@jpobst jpobst force-pushed the no-net472 branch 2 times, most recently from 46eb68b to 2ecd241 Compare May 16, 2023 19:56
</PropertyGroup>

<ItemGroup>
<Compile Include="..\utils\NullableAttributes.cs" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're removing use of NullableAttributes.cs, shouldn't we also delete src/utils/NullableAttributes.cs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few projects that must remain on netstandard2.0 because they are consumed by Xamarin.Android.Build.Tasks, which must remain netstandard2.0 so it can be loaded by VS MSBuild. These projects still need NullableAttributes.cs.

For example:

  • Java.Interop.Localization.csproj
  • Java.Interop.Tools.Diagnostics.csproj
  • Java.Interop.Tools.JavaCallableWrappers.csproj

@jonpryor jonpryor merged commit be2acbc into main May 18, 2023
@jonpryor jonpryor deleted the no-net472 branch May 18, 2023 15:00
jonpryor pushed a commit to dotnet/android that referenced this pull request May 19, 2023
Changes: dotnet/java-interop@dbb9edd...be2acbc

  * dotnet/java-interop@be2acbcb: [build] Remove use of 'net472' Target Framework (dotnet/java-interop#1099)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>8
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
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