Remove cecil submodule #597

jonpryor · 2020-03-10T00:24:50Z

Cecil is an "interesting" complication: it's a dependency of
Java.Interop, but Xamarin.Android use requires that it be "vendorized"
-- renamed as Xamarin.Android.Cecil.dll -- to avoid previously seen
issues because it's an unsigned assembly, and thus There Can Be Only
One Mono.Cecil.dll loaded into an AppDomain, and whichever is loaded
first "wins", but that version may not be compatible with what other
assemblies in the AppDomain need, and...

Renaming the assembly was just seen as the easiest solution.

This choice hasn't been without its own shortcomings; see e.g. commits
168c94d ("priorities"), cfa74d3 (downstream build system changes),
5eeb287 ("rebuilds are hard"), 6717275 ("because of 168c94d,
xamarin-android 'owns' the checkout, but that may not be API
compatible, oops"), etc., etc.

Plus, it kinda became moot with xamarin/xamarin-android@0c9f83b7
which removed the mono git submodule from xamarin-android. Instead
of building mono from source -- and, implicitly, building cecil from
source -- mono was instead obtained from a "mono archive" which
contained a prebuilt Mono.Cecil.dll which was "renamed" to
Xamarin.Android.Cecil.dll.

Which meant that in a xamarin-android build, cecil should never be
built from source anymore, which in turn meant that -- give or take
the occasional build system bug --
Java.Interop/src/Xamarin.Android.Cecil and company were "dead code",
as far as the commercial product is concerned.

Meanwhile, the existance of src/Xamarin.Android.Cecil proved to be
an ongoing source of maintenance pain, as -- depending on the IDE --
it couldn't build reliably, or would rebuild when it shouldn't have.

Rethink the whole Cecil relationship. If xamarin-android doesn't
require a cecil source checkout, why not ditch it entirely?

Remove the external/cecil git submodule, and the
src/Xamarin.Android.Cecil* projects, and replace them with NuGet
package references to the Mono.Cecil NuGet package.

What this means is that a "pure Java.Interop" build will now have
different assembly references than the "same" utilities built from
xamarin-android. For example, generator.exe, when built from
Java.Interop, will reference Mono.Cecil.dll, while it will instead
reference Xamarin.Android.Cecil.dll when built from xamarin-android.

The $(CecilSourceDirectory) MSBuild property is used to determine
whether the Mono.Cecil NuGet package or the Xamarin.Android.Cecil.dll
assembly reference should be used at build time.

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jonpryor · 2020-03-10T01:21:57Z

...and the xamarin-android side... dotnet/android#4376

DO NOT MERGE until dotnet/android#4376 is acceptable.

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jpobst · 2020-03-10T15:17:08Z

build-tools/scripts/cecil.projitems

+  <ItemGroup Condition=" '$(CecilSourceDirectory)' == '' ">
+    <PackageReference Include="Mono.Cecil" Version="0.11.2" />
+  </ItemGroup>
+  <ItemGroup Condition=" '$(CecilSourceDirectory)' != '' ">


I wonder if this condition should be:

Exists('$(UtilityOutputFullPath)Xamarin.Android.Cecil.dll')

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jpobst · 2020-03-10T18:32:42Z

/azp run

azure-pipelines · 2020-03-10T18:32:48Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

jpobst · 2020-03-10T18:35:11Z

There's a couple of now unneeded Cecil files here: https://github.com/xamarin/java.interop/tree/master/external

jonpryor · 2020-03-10T18:49:58Z

/azp run

azure-pipelines · 2020-03-10T18:50:04Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

jonpryor · 2020-03-10T19:44:02Z

Short-form SDK project migration needs to happen first: #547

jonpryor · 2020-03-17T20:26:58Z

PR #547 has been merged and this PR has been rebased.

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jpobst

❤️ I played with this for a bit locally in VS2019 and it seems great.

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works... Need to use `msbuild /restore` for all builds to allow `Java.Interop.Tools.Cecil` to build; see: dotnet#4376 (comment)

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works... Need to use `msbuild /restore` for all builds to allow `Java.Interop.Tools.Cecil` to build; see: dotnet#4376 (comment) Note: `$(TargetFrameworks)` throws a wrench into things. If two `$(TargetFramework)` builds share the same output directory, the `IncrementalClean` target will *remove files created by previous builds*, e.g. when e.g. `generator.csproj` builds the `netcoreapp3.1` framework, it will *delete* the `generator.exe` built by the `net472` framework, which results in subsequent build breaks. The only path to sanity is to *ensure* that different `$(TargetFramework)` builds have *completely separate* `$(OutputPath)` values. The "normal" approach to doing this is for `$(OutputPath)` to end with `$(TargetFramework)`. The problem here is that xamarin-android is setup so that we mirror the installation directory, with e.g. `bin/Debug` being an "installation root", and that installation root *won't* (normally?) have *both* net472 and netcoreapp3.1 outputs for the "same" project. The "solution" is to use the new `$(UtilityOutputFullPathCoreApps)` property within Java.Interop to ensure that Java.Interop "utility" apps -- normally placed into `bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android` -- are *instead* placed into `bin/Debug-netcoreapp3.1` for netcoreapp3.1 builds. This ensure that things *actually build*. Locally. YMMV.

Cecil is an "interesting" complication: it's a dependency of Java.Interop, but Xamarin.Android use requires that it be "vendorized" -- renamed as `Xamarin.Android.Cecil.dll` -- to avoid previously seen issues because it's an unsigned assembly, and thus There Can Be Only One `Mono.Cecil.dll` loaded into an AppDomain, and whichever is loaded first "wins", but that version may not be compatible with what other assemblies in the AppDomain need, and... Renaming the assembly was just seen as the easiest solution. This choice hasn't been without its own shortcomings; see e.g. commits 168c94d ("priorities"), cfa74d3 (downstream build system changes), 5eeb287 ("rebuilds are hard"), 6717275 ("because of 168c94d, xamarin-android 'owns' the checkout, but that may not be API compatible, oops"), etc., etc. Plus, it kinda became moot with [dotnet/android@0c9f83b7][0] which removed the `mono` git submodule from xamarin-android. Instead of building mono from source -- and, implicitly, building *cecil* from source -- mono was instead obtained from a "mono archive" which contained a prebuilt `Mono.Cecil.dll` which was "renamed" to `Xamarin.Android.Cecil.dll`. Which meant that in a xamarin-android build, cecil should *never* be built from source anymore, which in turn meant that -- give or take the occasional build system bug -- `Java.Interop/src/Xamarin.Android.Cecil` and company were "dead code", as far as the commercial product is concerned. Meanwhile, the existance of `src/Xamarin.Android.Cecil` proved to be an ongoing source of maintenance pain, as -- depending on the IDE -- it couldn't build reliably, or would rebuild when it shouldn't have. Rethink the whole Cecil relationship. If xamarin-android doesn't require a cecil source checkout, why not ditch it entirely? Remove the `external/cecil` git submodule, and the `src/Xamarin.Android.Cecil*` projects, and replace them with NuGet package references to the [`Mono.Cecil` NuGet package][1]. What this means is that a "pure Java.Interop" build will now have *different* assembly references than the "same" utilities built from xamarin-android. For example, `generator.exe`, when built from Java.Interop, will reference `Mono.Cecil.dll`, while it will instead reference `Xamarin.Android.Cecil.dll` when built from xamarin-android. The `$(CecilSourceDirectory)` MSBuild property is used to determine whether the Mono.Cecil NuGet package or the `Xamarin.Android.Cecil.dll` assembly reference should be used at build time. [0]: dotnet/android@0c9f83b [1]: https://www.nuget.org/packages/Mono.Cecil/0.11.2

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works... Need to use `msbuild /restore` for all builds to allow `Java.Interop.Tools.Cecil` to build; see: dotnet#4376 (comment) Note: `$(TargetFrameworks)` throws a wrench into things. If two `$(TargetFramework)` builds share the same output directory, the `IncrementalClean` target will *remove files created by previous builds*, e.g. when e.g. `generator.csproj` builds the `netcoreapp3.1` framework, it will *delete* the `generator.exe` built by the `net472` framework, which results in subsequent build breaks. The only path to sanity is to *ensure* that different `$(TargetFramework)` builds have *completely separate* `$(OutputPath)` values. The "normal" approach to doing this is for `$(OutputPath)` to end with `$(TargetFramework)`. The problem here is that xamarin-android is setup so that we mirror the installation directory, with e.g. `bin/Debug` being an "installation root", and that installation root *won't* (normally?) have *both* net472 and netcoreapp3.1 outputs for the "same" project. The "solution" is to use the new `$(UtilityOutputFullPathCoreApps)` property within Java.Interop to ensure that Java.Interop "utility" apps -- normally placed into `bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android` -- are *instead* placed into `bin/Debug-netcoreapp3.1` for netcoreapp3.1 builds. This ensure that things *actually build*. Locally. YMMV.

jonpryor requested a review from jpobst March 10, 2020 00:24

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 10, 2020

Bump to jonpryor/java.interop@4f55ace3

b022fd2

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jonpryor mentioned this pull request Mar 10, 2020

Bump to xamarin/Java.Interop/master@56c92c70 dotnet/android#4376

Merged

jonpryor added the do-not-merge label Mar 10, 2020

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 10, 2020

Bump to jonpryor/java.interop@4f55ace3

1a55380

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jonpryor force-pushed the jonp-cecil-nuget branch from 4f55ace to fde797d Compare March 10, 2020 15:01

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 10, 2020

Bump to jonpryor/java.interop@fde797dc

d9e3261

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jpobst reviewed Mar 10, 2020

View reviewed changes

jonpryor force-pushed the jonp-cecil-nuget branch from fde797d to f69ec21 Compare March 10, 2020 16:15

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 10, 2020

Bump to jonpryor/java.interop@f69ec216

71c17f8

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jonpryor force-pushed the jonp-cecil-nuget branch 2 times, most recently from 96cc1c2 to d9d8345 Compare March 10, 2020 18:47

jonpryor force-pushed the jonp-cecil-nuget branch from d9d8345 to 7345295 Compare March 10, 2020 18:56

jonpryor force-pushed the jonp-cecil-nuget branch from 7345295 to 931a97e Compare March 17, 2020 20:26

jonpryor removed the do-not-merge label Mar 17, 2020

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 18, 2020

Bump to jonpryor/java.interop@f69ec216

4c0580e

***DO NOT MERGE*** Context: dotnet/java-interop#597 Just testing to see if dotnet/java-interop#597 works...

jpobst approved these changes Mar 18, 2020

View reviewed changes

jonpryor force-pushed the jonp-cecil-nuget branch from 931a97e to ae85418 Compare March 19, 2020 19:46

jonpryor force-pushed the jonp-cecil-nuget branch from ae85418 to 789285f Compare March 20, 2020 18:53

jonpryor merged commit 56c92c7 into dotnet:master Mar 27, 2020

jpobst added this to the d16-7 milestone Apr 10, 2020

github-actions bot locked and limited conversation to collaborators Apr 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove cecil submodule #597

Remove cecil submodule #597

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

jpobst Mar 10, 2020

Uh oh!

jonpryor Mar 10, 2020

Uh oh!

jpobst commented Mar 10, 2020

Uh oh!

azure-pipelines bot commented Mar 10, 2020

Uh oh!

jpobst commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

azure-pipelines bot commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 17, 2020

Uh oh!

jpobst left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Remove cecil submodule #597

Remove cecil submodule #597

Uh oh!

Conversation

jonpryor commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

jpobst Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

jonpryor Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

jpobst commented Mar 10, 2020

Uh oh!

azure-pipelines bot commented Mar 10, 2020

Uh oh!

jpobst commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

azure-pipelines bot commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 10, 2020

Uh oh!

jonpryor commented Mar 17, 2020

Uh oh!

jpobst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants