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

[build] Stop producing Xamarin.Android.Cecil.dll #8489

Merged
merged 10 commits into from
Nov 20, 2023
Merged

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Nov 7, 2023

Fixes: #3132
Context: https://github.com/xamarin/monodroid/commit/b9349491a141b5265e3d48eec9af589c8c5cc793

The Xamarin.Android.Cecil.dll assembly was created to to avoid potential
conflicts and versioning issues with the copy of Cecil that Xamarin
Studio was using at the time. Considering our migration to .NET, and
how infrequently we change our Mono.Cecil reference these days, these
potential issues are likely no longer a concern.

Our unique Xamarin.Android.Cecil.dll copy has been removed in favor of
Mono.Cecil package references in all projects that depend on it.

Context: xamarin/monodroid@b934949

The Xamarin.Android.Cecil.dll assembly was created to to avoid potential
conflicts and versioning issues with the copy of Cecil that Xamarin
Studio was using at the time.  Considering our migration to .NET, and
how infrequently we change our Mono.Cecil reference these days, these
potential issues are likely no longer a concern.

Our unique Xamarin.Android.Cecil.dll copy has been removed in favor of
Mono.Cecil package references in all projects that depend on it.
@pjcollins pjcollins added the do-not-merge PR should not be merged. label Nov 9, 2023
@pjcollins
Copy link
Member Author

Adding the do-not-merge label as we will likely want some more external validation in the IDEs before merging this.

@pjcollins pjcollins marked this pull request as ready for review November 9, 2023 21:28
@pjcollins pjcollins requested a review from jpobst November 9, 2023 21:29
@dellis1972
Copy link
Contributor

@pjcollins the main concern around this was not our extensions, but other third party extensions like that Refectoring one. If they used a Cecil and were loaded first it would break out builds.
It would be useful to see if that is still an issue. It might not be.

@tondat any thoughts on this?

Comment on lines +145 to +146
<_MSBuildFiles Include="$(MicrosoftAndroidSdkOutDir)Mono.Cecil.dll" />
<_MSBuildFiles Include="$(MicrosoftAndroidSdkOutDir)Mono.Cecil.pdb" />
Copy link
Member

Choose a reason for hiding this comment

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

My first concern was that MAUI ships Mono.Cecil, will we conflict?

So, I downloaded the package and inspected it: https://www.nuget.org/packages/Mono.Cecil/0.11.4

It looks to be strong named and appropriately versioned:

image

So, what may happen is that if MAUI ships Mono.Cecil, we may use theirs or vice-versa if the version & strong name key match. Although, it will likely "just work" if the version is identical.

I don't think there is a problem here -- it should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem used to be with VS extensions. It was never with other products like iOS etc.

Copy link
Member

Choose a reason for hiding this comment

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

.NET framework should know how to respect strong named + versioned assemblies and load new instances of them.

VS Mac would have had an issue on Mono, but I believe it will be out of support and never be able to build .NET 9 projects. VS Mac also made it to .NET 6, in its latest version?

@jpobst
Copy link
Contributor

jpobst commented Nov 9, 2023

Agreed that this feels like it could potentially conflict with other copies of Mono.Cecil.dll loaded into the VSWin process.

But it also feels like strong naming should protect us from that? Unless someone else makes local changes to their copy and doesn't change the version number?

@jpobst
Copy link
Contributor

jpobst commented Nov 13, 2023

Related: #3132

@pjcollins pjcollins removed the do-not-merge PR should not be merged. label Nov 16, 2023
@pjcollins
Copy link
Member Author

I spent some time testing in a Windows VM today and had no issues with some somewhat trivial build, deploy, debug, and hot reload scenarios with Android and MAUI apps using a VS 17.9 preview and WSA. I think we should be able to merge this, but am wondering if it makes sense to wait for the .NET 9 PR to land first @jonathanpeppers ?

@jonathanpeppers
Copy link
Member

I don't think we should wait on the .NET 9 PR to merge this.

It might be better to merge sooner than later, to find out if find any possible issue loading different Mono.Cecil.dll files.

@jonpryor
Copy link
Member

@pjcollins: please also try removing $(CecilSourceDirectory):

<CecilSourceDirectory>$(MSBuildThisFileDirectory)..\..\external\mono\external\cecil</CecilSourceDirectory>

$(CecilSourceDirectory) is used in Java.Interop to get the Java.Interop builds to use Xamarin.Android.Cecil.dll:

It doesn't look like removing $(CecilSourceDirectory) is required -- the Exists() check in TargetFrameworkDependentValues.targets will work properly if it's set and Xamarin.Android.Cecil.dll doesn't exist -- but I think it would be useful cleanup.

@jonpryor jonpryor merged commit 8f33823 into main Nov 20, 2023
48 checks passed
@jonpryor jonpryor deleted the dev/pjc/noxacecil branch November 20, 2023 18:36
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 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.

Try to stop renaming the Mono.Cecil.dll to Xamarin.Android.Cecil.dll
5 participants