Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 19, 2019

Convert these projects to SDK style .csproj files:

  • samples/Hello/Hello.csproj
  • tools/class-parse/class-parse.csproj
  • tools/generator/generator.csproj
  • tools/jcw-gen/jcw-gen.csproj
  • tools/jnimarshalmethod-gen/Xamarin.Android.Tools.JniMarshalMethodGenerator.csproj
  • tools/logcat-parse/logcat-parse.csproj

@pjcollins
Copy link
Member

pjcollins commented Mar 3, 2020

This has now produced a (mostly, aside from known issues) green build in XA - dotnet/android#4347, and is ready for further cleanup/review.

I'm a touch worried about extending our usage of <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath> given the issues we've seen in the past related to copying multi-targeted assemblies over each other, and will look to see if we can make some changes there.

@pjcollins pjcollins marked this pull request as ready for review March 3, 2020 22:11
@jpobst
Copy link
Contributor Author

jpobst commented Mar 3, 2020

Nice!

@pjcollins
Copy link
Member

There's still some work needed on the xamarin-android side before we can land this (specifically with respect to handling the addition of a target framework on certain output paths), but I'd love some more eyes on this since the diff is so large.

@jpobst
Copy link
Contributor Author

jpobst commented Mar 9, 2020

It feels like we are removing a lot of <AppendTargetFrameworkToOutputPath> for libraries that are not multi-targeted, and then adding a lot of complexity to try to handle the resulting paths.

I think we're better off using <AppendTargetFrameworkToOutputPath> unless there is a concrete issue it causes.

@pjcollins
Copy link
Member

It is more of a theoretical concern, though we did see issues related to this on the xamarin-android side with LibZipSharp.

One example of similar potential concern here is with HtmlAgilityPack, as it is referenced by both generator.csproj and Xamarin.Android.Tools.AnnotationSupport.csproj. These projects target net472 and netstandard2.0. Which version of HtmlAgilityPack.dll should end up in the output directory? Which version should end up in our installers? Presumably the netstandard version should take priority, but depending on project build ordering this assembly could be unexpectedly overwritten.

Additionally, once this "modernization" pr lands, we'll be looking at updating some of these tools to be .NET Core apps, and I think the <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath> removals will help with that effort. Maybe we should only remove this property on projects that produce an .exe for now though?

@jpobst
Copy link
Contributor Author

jpobst commented Mar 9, 2020

We could use something like this to ensure we choose the netstandard2.0 version of HtmlAgilityPack: https://duanenewman.net/blog/post/a-better-way-to-override-references-with-packagereference/

I can see leaving the TF for .exe files, since presumably we'll soon be building them with net471 and net-5, but for .dll's it would be nice it we could use <AppendTargetFrameworkToOutputPath>.

@pjcollins
Copy link
Member

I'm not sure if that explicit hint path approach would solve the problem entirely, and it doesn't handle the "which one do we use at runtime and redistribute" question. I can partially revert the <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath> removals though if that's the preferred path forward here. @jonpryor do you have any thoughts/preferences wrt these OutputPath related changes?

@jpobst
Copy link
Contributor Author

jpobst commented Mar 9, 2020

With the HintPath, only the one mentioned would end up in the OutputPath, so it will ensure we only use/ship the one we want.

@pjcollins
Copy link
Member

pjcollins commented Mar 9, 2020

Ok cool, that approach does appear to be working in a small test case as long as ExcludeAssets="Compile" is set on the package reference, so this could be a viable approach to ensure that the "correct" NuGet assets are copied to a single output dir.

It also has the added benefit of copying supplemental (.pdb/.xml) files automatically, without needing to set <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>. This would allow us to remove the new _CopyGeneratorPackageRefPdbsToOutputDirectory target in the generator project as well.

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.

Does this need a draft PR on the xamarin-android side to make sure it won't break the build?

@pjcollins
Copy link
Member

Yeah I've been testing with XA along the way in dotnet/android#4347. The latest changes are incompatible, but I will be pushing something here shortly that reworks things based on yesterdays discussion.

@jonpryor jonpryor mentioned this pull request Mar 10, 2020
@pjcollins
Copy link
Member

I think this is ready to go again after backing out the AppendTF removals. There's two failures in the latest XA build but they don't seem related - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3546904&view=ms.vss-test-web.build-test-results-tab&runId=11800708&resultId=100261&paneView=attachments

@dellis1972
Copy link
Contributor

@pjcollins I think I agree the actual error was

arm-linux-androideabi-strip: unable to rename 'obj\Release\aot\armeabi-v7a\libaot-UnnamedProject.dll.so.tmp'; reason: File exists 

which is really weird.

@jpobst
Copy link
Contributor Author

jpobst commented Mar 13, 2020

Nice work! ❤️

I approve, but since I'm technically the PR owner I can't add an approval.

@jonpryor jonpryor merged commit cedf4d0 into master Mar 17, 2020
@jonpryor jonpryor deleted the more-sdk branch March 17, 2020 19:24
jonpryor pushed a commit that referenced this pull request Mar 18, 2020
Bulk translate various `.csproj` files to [Short-Form Projects][0]:

  * Remove `packages.config`, replace with `@(PackageReference)`.

  * Set `$(AppendTargetFrameworkToOutputPath)`=False so that we
    preserve the existing directory structure.

  * Remove `Mono.Options-PCL.cs`, replaced by `Mono.Options` NuGet.

  * `JdkInfo.props` doesn't exist when doing preparation via the
    xamarin-android repo, so ignore it if it doesn't exist yet.

[0]: https://docs.microsoft.com/en-us/dotnet/core/tools/csproj#additions

Co-authored-by: Peter Collins <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 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.

7 participants