Skip to content

[java-source-utils] Build one $(TargetFramework) #1007

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

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Jul 7, 2022

Context: dotnet/android#7157

Ever since commit 69e1b80, java-source-utils.csproj used the
$(TargetFrameworks) plural form, even though it only specified a
single framework. I don't remember underlying reason for this, other
than "that's what I needed for it to build," which might have been
because the _BuildJava target was "wrong".

This arrangement was "fine", until dotnet/android@2197a459,
after which the xamarin-android/main CI builds started behaving
"weird": frequently make all-tests would fail, because
make jenkins would produce an invalid or corrupt
java-source-utils.jar, apparently because it was attempting to
concurrently build java-source-utils.csproj (?!).

One of these concurrent builds appeared to be an "outer build" from
Xamarin.Android.sln, while another one appeared to be an "inner
build" via apksigner.csproj and
%(ProjectReference.AdditionalProperties):

17:53:44.526 59:11>Target "_BuildJava: (TargetId:490)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
17:53:44.526 59:11>Building target "_BuildJava" completely.
  Output file "/Users/runner/work/1/s/xamarin-android/bin/Release/lib/packs/Microsoft.Android.Sdk.Darwin/33.0.0/tools/java-source-utils.jar" does not exist.
…
17:54:19.564 59:12>Target "DispatchToInnerBuilds: (TargetId:1637)" in file "/Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/sdk/7.0.100-preview.7.22354.2/Microsoft.Common.CrossTargeting.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (target "Build" depends on it):
  Task "MSBuild" (TaskId:1099)
    Task Parameter:BuildInParallel=True (TaskId:1099)
    Task Parameter:Targets=Build (TaskId:1099)
    Task Parameter:
        Projects=
            java-source-utils.csproj
                    AdditionalProperties=TargetFramework=net7.0 (TaskId:1099)
    Additional Properties for project "java-source-utils.csproj": (TaskId:1099)
      TargetFramework=net7.0 (TaskId:1099)
…
17:54:19.592 59:12>Target "_BuildJava: (TargetId:1640)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
  Building target "_BuildJava" completely.
  Output file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/../../bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar" does not exist.
…
17:54:41.034 59:11>Done building target "_BuildJava" in project "java-source-utils.csproj".: (TargetId:490)

We attempted to fix this by removing
%(ProjectReference.AdditionalProperties), which only slightly
changed things: the "outer build via Xamarin.Android.sln" build was
removed, but we instead saw a scenario in which
java-source-utils.csproj was built "once", and as part of that
build the "outer" and "inner" builds were run concurrently.

A commonality here is $(TargetFrameworks) requires "outer" and
"inner" builds, and that is a complication that we should remove.

Update java-source-utils.csproj so that $(TargetFramework) is
used, not $(TargetFrameworks).

Update the _BuildJava target so that it runs before the
GetCopyToOutputDirectoryItems target. This is consistent with how
the _BuildGradle target in apksigner.csproj works. Without
this change -- and the removal of the empty Build target -- the
java-source-utils.csproj build didn't behave correctly (gradlew
wasn't run).

Unfortunately, this seemingly simple change hits a little "snag":
Directory.Build.props is imported very early:

Directory.Build.props is imported very early in
Microsoft.Common.props, and properties defined later are
unavailable to it.

"Properties defined later are unavailable to it." Properties such as
$(TargetFramework). Which means that every property we have in
Directory.Build.props which "depend" on $(TargetFramework)
are not actually usable. They've only appeared to work
because they would default to "classic" paths, but as soon as you try
to build with $(TargetFramework)=net7.0 -- as was attempted with
java-source-utils.csproj, and previously with Java.Base.csproj
(bc5bcf4) -- you'll find that the "wrong" directories are used for
$(OutputPath).

This has been a long-standing deficiency in my MSBuild understanding.

Fix this by adding a new TargetFrameworkDependentValues.props file,
and <Import/>ing this file in every .csproj which sets MSBuild
properties with values dependent upon $(TargetFramework) before
those properties are set.

Old and busted:

<PropertyGroup>
  <TargetFramework>net7.0</TargetFramework>
  <OutputPath>$(UtilityOutputFullPath)</OutputPath>
</PropertyGroup>

New hawtness:

<PropertyGroup>
  <TargetFramework>net7.0</TargetFramework>
</PropertyGroup>
<Import Project="..\..\TargetFrameworkDependentValues.props" />
<PropertyGroup>
  <OutputPath>$(UtilityOutputFullPath)</OutputPath>
</PropertyGroup>

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Jul 7, 2022
@jonpryor jonpryor force-pushed the jonp-jsu-single-tf branch 2 times, most recently from 8d0836d to 46901ab Compare July 7, 2022 21:06
Context: dotnet/android#7157

Ever since commit 69e1b80, `java-source-utils.csproj` used the
`$(TargetFrameworks)` plural form, even though it only specified a
single framework.  I don't remember underlying reason for this, other
than "that's what I needed for it to build," which might have been
because the `_BuildJava` target was "wrong".

This arrangement was "fine", until dotnet/android@2197a459,
after which the xamarin-android/main CI builds started behaving
"weird": frequently `make all-tests` would fail, because
`make jenkins` would produce an invalid or corrupt
`java-source-utils.jar`, *apparently* because it was attempting to
*concurrently build* `java-source-utils.csproj` (?!).

One of these concurrent builds *appeared* to be an "outer build" from
`Xamarin.Android.sln`, while another one appeared to be an "inner
build" via `apksigner.csproj` and
`%(ProjectReference.AdditionalProperties)`:

	17:53:44.526 59:11>Target "_BuildJava: (TargetId:490)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
	17:53:44.526 59:11>Building target "_BuildJava" completely.
	  Output file "/Users/runner/work/1/s/xamarin-android/bin/Release/lib/packs/Microsoft.Android.Sdk.Darwin/33.0.0/tools/java-source-utils.jar" does not exist.
	…
	17:54:19.564 59:12>Target "DispatchToInnerBuilds: (TargetId:1637)" in file "/Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/sdk/7.0.100-preview.7.22354.2/Microsoft.Common.CrossTargeting.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (target "Build" depends on it):
	  Task "MSBuild" (TaskId:1099)
	    Task Parameter:BuildInParallel=True (TaskId:1099)
	    Task Parameter:Targets=Build (TaskId:1099)
	    Task Parameter:
	        Projects=
	            java-source-utils.csproj
	                    AdditionalProperties=TargetFramework=net7.0 (TaskId:1099)
	    Additional Properties for project "java-source-utils.csproj": (TaskId:1099)
	      TargetFramework=net7.0 (TaskId:1099)
	…
	17:54:19.592 59:12>Target "_BuildJava: (TargetId:1640)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
	  Building target "_BuildJava" completely.
	  Output file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/../../bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar" does not exist.
	…
	17:54:41.034 59:11>Done building target "_BuildJava" in project "java-source-utils.csproj".: (TargetId:490)

We attempted to fix this by removing
`%(ProjectReference.AdditionalProperties)`, which only slightly
changed things: the "outer build via `Xamarin.Android.sln`" build was
removed, but we instead saw a scenario in which
`java-source-utils.csproj` was built "once", and as part of that
build the "outer" and "inner" builds were run concurrently.

A commonality here is `$(TargetFrameworks)` requires "outer" and
"inner" builds, and that is a complication that we should remove.

Update `java-source-utils.csproj` so that `$(TargetFramework)` is
used, not `$(TargetFrameworks)`.

Update the `_BuildJava` target so that it runs before the
`GetCopyToOutputDirectoryItems` target.  This is consistent with how
the [`_BuildGradle` target in `apksigner.csproj`][0] works.  Without
this change -- and the removal of the empty `Build` target -- the
`java-source-utils.csproj` build didn't behave correctly (`gradlew`
wasn't run).

Unfortunately, this seemingly simple change hits a little "snag":
`Directory.Build.props` is imported [very early][1]:

> *Directory.Build.props* is imported very early in
> *Microsoft.Common.props*, and properties defined later are
> unavailable to it.

"Properties defined later are unavailable to it." Properties such as
`$(TargetFramework)`.  Which means that every property we have in
`Directory.Build.props` which "depend" on `$(TargetFramework)`
*are not **actually** usable*.  They've only *appeared* to work
because they would default to "classic" paths, but as soon as you try
to build with `$(TargetFramework)`=net7.0 -- as was attempted with
`java-source-utils.csproj`, and previously with `Java.Base.csproj`
(bc5bcf4) -- you'll find that the "wrong" directories are used for
`$(OutputPath)`.

This has been a long-standing deficiency in my MSBuild understanding.

Fix this by adding a new `TargetFrameworkDependentValues.props` file,
and `<Import/>`ing this file in *every* `.csproj` which sets MSBuild
properties with values dependent upon `$(TargetFramework)` *before*
those properties are set.

Old and busted:

	<PropertyGroup>
	  <TargetFramework>net7.0</TargetFramework>
	  <OutputPath>$(UtilityOutputFullPath)</OutputPath>
	</PropertyGroup>

New hawtness:

	<PropertyGroup>
	  <TargetFramework>net7.0</TargetFramework>
	</PropertyGroup>
	<Import Project="..\..\TargetFrameworkDependentValues.props" />
	<PropertyGroup>
	  <OutputPath>$(UtilityOutputFullPath)</OutputPath>
	</PropertyGroup>

[0]: https://github.com/xamarin/xamarin-android/blob/c537dd28c30f482f365ef756214be35aa1553da2/src/apksigner/apksigner.targets#L3-L13
[1]: https://docs.microsoft.com/en-us/visualstudio/msbuild/customize-your-build?view=vs-2022#import-order
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I just had a couple minor comments.

I think what you did with TargetFrameworkDependentValues.props is probably the best option. I couldn't think of another MSBuild feature that would help this case.

</PropertyGroup>

<PropertyGroup Condition=" '$(JIBuildingForNetCoreApp)' == 'True' ">
<IntermediateOutputPath>$(BaseIntermediateOutputPath)\$(Configuration)-$(TargetFramework.ToLowerInvariant())</IntermediateOutputPath>
Copy link
Member

Choose a reason for hiding this comment

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

You might put a trailing \ on this one. The .NET SDK might fix it for you depending on ordering.

<ProduceReferenceAssembly>true</ProduceReferenceAssembly>
<MSBuildWarningsAsMessages>NU1702</MSBuildWarningsAsMessages>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an old thing VS would do. Could we update it to:

Suggested change
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
<PropertyGroup Condition=" '$(Configuration)' == 'Debug' ">

@jonpryor jonpryor merged commit c942ab6 into dotnet:main Jul 8, 2022
@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