-
Notifications
You must be signed in to change notification settings - Fork 528
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
Conversation
f2a63b0
to
c1b4267
Compare
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be overridable?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be $(_IsXBuild)
?
There was a problem hiding this comment.
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'" |
There was a problem hiding this comment.
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?
c1b4267
to
6663746
Compare
build |
public App() | ||
{ | ||
JsonConvert.DeserializeObject<string>(""test""); | ||
var package = Package.Open (""""); |
There was a problem hiding this comment.
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. :-/
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
6663746
to
06d1f4b
Compare
06d1f4b
to
47ef4b3
Compare
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.
Changes: xamarin/monodroid@45abd3f...ed584c7 * xamarin/monodroid@ed584c775: [tools/msbuild] Improve FastDeploy error messages (dotnet#1190) * xamarin/monodroid@74294dca0: Bump to xamarin/androidtools@47ec118f (dotnet#1195) * xamarin/monodroid@ff633623e: [tools/msbuild] Add additional diagnostic logging for FastDev. (dotnet#1194) * xamarin/monodroid@546d5fb58: [tools/msbuild] Add Better Diagnostic Logging for Fast Deployment. (dotnet#1185) * xamarin/monodroid@fa6a1a058: [tools/msbuild] incude PackageName in MAX_COMMAND adb check. (dotnet#1189) * xamarin/monodroid@ac447cb5d: Bump Xamarin.Android to bring in JDK Fixes (dotnet#1192)
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.)
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.