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

Better NetStandard Support #1185

Merged
merged 5 commits into from Feb 27, 2018
Merged

Better NetStandard Support #1185

merged 5 commits into from Feb 27, 2018

Conversation

dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Jan 11, 2018

Context #1154

This PR brings in changes from xamarin/xamarin-macios#2643 and xamarin/xamarin-macios#2731 to improve our net standard support. While it does not fix the packaging problem in #1154 it will give us parity with the iOS code base.

@dellis1972 dellis1972 added do-not-merge PR should not be merged. Area: App+Library Build Issues when building Library projects or Application projects. labels Jan 11, 2018
@dellis1972 dellis1972 changed the title Issue1154 [WIP] Better NetStandard Support Feb 19, 2018
@dellis1972 dellis1972 removed the do-not-merge PR should not be merged. label Feb 19, 2018
@@ -3,6 +3,7 @@
<XamarinAndroidVersion>@PACKAGE_VERSION@-@PACKAGE_VERSION_BUILD@</XamarinAndroidVersion>
<_JavaInteropReferences>Java.Interop;System.Runtime</_JavaInteropReferences>
<DependsOnSystemRuntime Condition=" '$(DependsOnSystemRuntime)' == '' ">true</DependsOnSystemRuntime>
<ImplicitlyExpandNETStandardFacades>false</ImplicitlyExpandNETStandardFacades>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be overridable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a common (maybe "de facto" standard) MSBuild property to control whether to include those references or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the iOS/Mac tooling does not allow this to be overridable. I suspect this is by design so NetStandard Facades are never included in the output.

@@ -2,6 +2,7 @@
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<!-- PCL Support -->
<PropertyGroup>
<IsXBuild Condition="'$(MSBuildRuntimeVersion)' == ''">true</IsXBuild>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be $(_IsXBuild)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup it should


<UsingTask
TaskName="GetDependsOnNETStandard"
Condition="'$(IsXBuild)' != 'true'"
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be Conditional on the AssemblyFile existing?

@dellis1972
Copy link
Contributor Author

build

public App()
{
JsonConvert.DeserializeObject<string>(""test"");
var package = Package.Open ("""");
Copy link
Member

Choose a reason for hiding this comment

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

Wait, what? What does """" mean? csharp doesn't like that:

$ csharp
csharp> string s = ""test"";
(1,14): error CS1525: Unexpected symbol `test'

I don't understand how this and the previous line even work. :-/

Copy link
Member

Choose a reason for hiding this comment

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

The use of var here means the App.package isn't being set, as it is hidden. Is App.package actually needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code block is part of a string @"" block [1]. So the """" are escaped double quotes. We need a variable to use Package otherwise the linker will strip the assembly from the final apk. This way we can make sure the required System.IO.Packaging assembly ends up in the .apk (for the test).

[1] https://github.com/dellis1972/xamarin-android/blob/6663746918fc2cd255c913d45092c9d7b1ec76d8/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/PackagingTest.cs#L265

Copy link
Member

Choose a reason for hiding this comment

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

We need a variable to use Package otherwise the linker will strip the assembly from the final apk.

I still don't understand. We need a call to Package.Open() to preserve the type; that makes sense. That doesn't answer why the App.package field exists, nor does it answer why the App.package field is ignored. App.package will always be null, because the var package declaration within the App constructor hides the App.package field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah right , sorry I get ya now lol

@jonpryor jonpryor merged commit f988952 into xamarin:master Feb 27, 2018
jonpryor pushed a commit that referenced this pull request Feb 27, 2018
Context: #1154

This PR brings in changes from xamarin/xamarin-macios#2643 and
xamarin/xamarin-macios#2731 to improve our .NET Standard support.
While this does not fix the packaging problem in #1154 it will give
us parity with the iOS code base.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Apr 21, 2021
Changes: xamarin/monodroid@45abd3f...ed584c7

  * xamarin/monodroid@ed584c775: [tools/msbuild] Improve FastDeploy error messages (xamarin#1190)
  * xamarin/monodroid@74294dca0: Bump to xamarin/androidtools@47ec118f (xamarin#1195)
  * xamarin/monodroid@ff633623e: [tools/msbuild] Add additional diagnostic logging for FastDev. (xamarin#1194)
  * xamarin/monodroid@546d5fb58: [tools/msbuild] Add Better Diagnostic Logging for Fast Deployment. (xamarin#1185)
  * xamarin/monodroid@fa6a1a058: [tools/msbuild] incude PackageName in MAX_COMMAND adb check. (xamarin#1189)
  * xamarin/monodroid@ac447cb5d: Bump Xamarin.Android to bring in JDK Fixes (xamarin#1192)
jonpryor pushed a commit that referenced this pull request Apr 22, 2021
Fixes: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1301607

Context: #5710
Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1290661

Changes: xamarin/monodroid@45abd3f...ff63362

  * xamarin/monodroid@ff633623e: [tools/msbuild] Add additional diagnostic logging for FastDev. (#1194)
  * xamarin/monodroid@546d5fb58: [tools/msbuild] Add Better Diagnostic Logging for Fast Deployment. (#1185)
  * xamarin/monodroid@fa6a1a058: [tools/msbuild] incude PackageName in MAX_COMMAND adb check. (#1189)
  * xamarin/monodroid@ac447cb5d: Bump Xamarin.Android to bring in JDK Fixes (#1192)

Issue #5710 was an error in Fast Deployment when an app had "lots" of
localization assemblies, causing an `adb shell` command to become
"too long".  This was fixed in xamarin/monodroid@3a05726b and d9ed129.

Add a new unit test which contains *all* available languages, as
returned by `CultureInfo.GetCultures(CultureTypes.AllCultures)`
(possibly 342 of them!).

(This new test found that there were additional issues around
Fast Deployment and "lots" of localization assemblies, addressed in
xamarin/monodroid@fa6a1a058.)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants