[msbuild] Add reference to `System.Drawing.Common.dll` to XI projects. #6011

radical · 2019-05-08T04:04:16Z

@akoeplinger: Since we moved types from Mono.Android.dll and
Xamarin.iOS/WatchOS/TVOS.dll to System.Drawing.Common.dll user projects
would fail to compile. We need to add some msbuild logic to add a
reference to the assembly automatically.

@akoeplinger

Fixes mono/mono#13483 : ``` @akoeplinger: Since we moved types from Mono.Android.dll and Xamarin.iOS/WatchOS/TVOS.dll to System.Drawing.Common.dll user projects would fail to compile. We need to add some msbuild logic to add a reference to the assembly automatically. ```

rolfbjarne

I tested this on iOS, tvOS and watchOS projects, and the fix worked.

I also tested on Xamarin.Mac projects, and the fix didn't work there, so I copied the fix to the corresponding Xamarin.Mac targets file, and that worked, so I pushed the change.

@radical is there any way to for customers to undo/ignore this fix locally (in their csproj) if it ends up causing problems for someone?

monojenkins · 2019-05-08T09:53:01Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

2 tests failed, 0 tests skipped, 99 tests passed.

Failed tests

MSBuild tests/iOS: Failed (Execution failed with exit code 2)
mmptest/macOS/NonSystem: Failed (Execution failed with exit code 1)

…s_* tests. We're including a new assembly, which means the Xamarin.iOS.Tasks.TargetTests.GetReferencedAssemblies_* must be updated accordingly. Also modify these tests so that test assert that fails lists the actual assembly that's missing, i.e. instead of this: 1) Test Failure : Xamarin.iOS.Tasks.TargetTests.GetReferencedAssemblies_Executable xamarin#1 Expected: 6 But was: 7 we now print: 1) Test Failure : Xamarin.iOS.Tasks.TargetTests.GetReferencedAssemblies_Executable References Expected: equivalent to < "mscorlib.dll", "MyLibrary.dll", "System.Core.dll", "System.dll", "System.Xml.dll", "Xamarin.iOS.dll" > But was: < "mscorlib.dll", "MyLibrary.dll", "System.Core.dll", "System.dll", "System.Drawing.Common.dll", "System.Xml.dll", "Xamarin.iOS.dll" >

spouliot · 2019-05-08T12:33:36Z

I think we should remove our hacks so we'll have an earlier warning is it breaks.

git grep System.Drawing.Common tests/
tests/introspection/iOS/introspection-ios.csproj:    <Reference Include="System.Drawing.Common" /> <!-- FIXME: https://github.com/mono/mono/issues/13483 -->
tests/monotouch-test/monotouch-test.csproj:    <Reference Include="System.Drawing.Common" /> <!-- FIXME: https://github.com/mono/mono/issues/13483 -->
tests/xammac_tests/xammac_tests.csproj:    <Reference Include="System.Drawing.Common" /> <!-- FIXME: https://github.com/mono/mono/issues/13483 -->

…owReference_ToSystemDrawing. The test was verifying that referencing System.Drawing.dll and trying to use System.Drawing.RectangleF would fail to compile (because System.Drawing.dll shouldn't be resolved in this case). The addition of System.Drawing.Common.dll breaks this assumption, because now we ship System.Drawing.RectangleF, so the code that was supposed to fail to compile works just fine instead. So modify the test to verify that there's no System.Drawing.dll in the final bundle.

rolfbjarne · 2019-05-08T12:42:17Z

I think we should remove our hacks so we'll have an earlier warning is it breaks.

Good point, I've removed them.

radical · 2019-05-08T17:06:56Z

@radical is there any way to for customers to undo/ignore this fix locally (in their csproj) if it ends up causing problems for someone?

@rolfbjarne Not as is, but this could be made conditional on some new property that the user can set to disable this behavior.

spouliot · 2019-05-08T18:29:42Z

bot issues

Java NullPointerException in logThis likely indicates an error in the Jenkins slave processIndication 1
--
  | MicrosoftAzureStorage - Upload ErrorMicrosoftAzureStorage - Error occurred while uploading to AzureIndication 2

spouliot · 2019-05-08T18:30:12Z

build

monojenkins · 2019-05-09T02:16:53Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

6 tests failed, 0 tests skipped, 228 tests passed.

Failed tests

dont link/watchOS - simulator/Release: Crashed
[NUnit] Mono SystemXmlTests/watchOS - simulator/Debug: Crashed
[NUnit] Mono SystemDataTests/watchOS - simulator/Debug: Crashed
[xUnit] Mono SystemSecurityXunit/watchOS - simulator/Debug: Crashed
[xUnit] Mono SystemXmlXunit/watchOS - simulator/Debug: Crashed
mmptest/macOS/NonSystem: Failed (Execution failed with exit code 1)

…g.Common.dll causes problems.

spouliot · 2019-05-09T12:40:56Z

6 tests failed, 0 tests skipped, 228 tests passed
5x watchOS failures due to HE0038 https://github.com/xamarin/maccore/issues/581
1x failure due to expired certificates https://github.com/xamarin/maccore/issues/1597

rolfbjarne · 2019-05-09T12:44:47Z

this could be made conditional on some new property that the user can set to disable this behavior.

Ok, I've added a DisableAutomaticSystemDrawingCommonReference variable that can be set (if anyone has a better name I'm open to suggestions).

emaf · 2019-05-09T13:18:06Z

msbuild/Xamarin.iOS.Tasks.Core/Xamarin.iOS.Common.targets

@@ -458,6 +458,13 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
 		<Delete SessionId="$(BuildSessionId)" Condition="'$(IsMacEnabled)' == 'true'" Files="@(_IpaPackageFile)" />
 	</Target>

+	<Target Name="_AddExtraReferences" BeforeTargets="ResolveAssemblyReferences" Condition="'$(DisableAutomaticSystemDrawingCommonReference)' == ''">


Same thing than in the Mac common targets

emaf · 2019-05-09T13:18:13Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.targets

@@ -186,6 +186,13 @@ Copyright (C) 2014 Xamarin. All rights reserved.
 		<RemoveDir Directories="$(IntermediateOutputPath)" />
 	</Target>

+	<Target Name="_AddExtraReferences" BeforeTargets="ResolveAssemblyReferences" Condition="'$(DisableAutomaticSystemDrawingCommonReference)' == ''">


I think DisableAutomaticSystemDrawingCommonReference should have a generic name to match the target name like DisableExtraReferences or DisableAutomaticExtraReferences.

Besides that, since it's a boolean property it'd be better give it a default value if it's empty (set it to false) or check if it's != 'true', because if someone sets it to false for any reason the target will be skipped when it shouldn't.

@emaf ok, I've changed it to use DisableExtraReferences and check for (not) true instead of empty string.

monojenkins · 2019-05-09T21:54:44Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded

rolfbjarne · 2019-05-09T23:21:03Z

@monojenkins backport d16-2

radical requested review from spouliot, akoeplinger and chamons May 8, 2019 04:04

radical requested review from emaf, jstedfast, kzu, rolfbjarne and VincentDondain as code owners May 8, 2019 04:04

radical mentioned this pull request May 8, 2019

Adding System.Drawing.Common.dll reference for XI/XA projects mono/mono#13483

Closed

[msbuild] Implement the same fix for XM projects as well.

6f70dbf

rolfbjarne approved these changes May 8, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

spouliot approved these changes May 8, 2019

View reviewed changes

rolfbjarne added 2 commits May 8, 2019 14:37

Remove workarounds for mono/mono#13483.

5bafdd4

[msbuild] Create a way out if automatically referencing System.Drawin…

bae0abb

…g.Common.dll causes problems.

dalexsoto approved these changes May 9, 2019

View reviewed changes

emaf approved these changes May 9, 2019

View reviewed changes

emaf requested changes May 9, 2019

View reviewed changes

[msbuild] Adjust variable name and boolean logic according to review.

d7838a3

emaf approved these changes May 9, 2019

View reviewed changes

spouliot mentioned this pull request May 9, 2019

[d16-2] Issues tracked for release #6033

Closed

5 tasks

chamons approved these changes May 9, 2019

View reviewed changes

rolfbjarne merged commit 3a7bdc0 into xamarin:master May 9, 2019

monojenkins mentioned this pull request May 9, 2019

[d16-2] [msbuild] Add reference to System.Drawing.Common.dll to XI projects. #6035

Merged

radical deleted the add-drawing-common-reference-master branch May 10, 2019 02:51

chamons mentioned this pull request Oct 17, 2019

System.Drawing.Common Gets Added DLL & To NuGet on Pack #7249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[msbuild] Add reference to `System.Drawing.Common.dll` to XI projects. #6011

[msbuild] Add reference to `System.Drawing.Common.dll` to XI projects. #6011

radical commented May 8, 2019

rolfbjarne left a comment

This comment has been minimized.

monojenkins commented May 8, 2019

Failed tests

spouliot commented May 8, 2019

rolfbjarne commented May 8, 2019

radical commented May 8, 2019

spouliot commented May 8, 2019

spouliot commented May 8, 2019

monojenkins commented May 9, 2019

Failed tests

spouliot commented May 9, 2019

rolfbjarne commented May 9, 2019

emaf May 9, 2019

emaf May 9, 2019

rolfbjarne May 9, 2019

monojenkins commented May 9, 2019

rolfbjarne commented May 9, 2019

[msbuild] Add reference to System.Drawing.Common.dll to XI projects. #6011

[msbuild] Add reference to System.Drawing.Common.dll to XI projects. #6011

Conversation

radical commented May 8, 2019

rolfbjarne left a comment

Choose a reason for hiding this comment

This comment has been minimized.

monojenkins commented May 8, 2019

Test results

Failed tests

spouliot commented May 8, 2019

rolfbjarne commented May 8, 2019

radical commented May 8, 2019

spouliot commented May 8, 2019

spouliot commented May 8, 2019

monojenkins commented May 9, 2019

Test results

Failed tests

spouliot commented May 9, 2019

rolfbjarne commented May 9, 2019

emaf May 9, 2019

Choose a reason for hiding this comment

emaf May 9, 2019

Choose a reason for hiding this comment

rolfbjarne May 9, 2019

Choose a reason for hiding this comment

monojenkins commented May 9, 2019

rolfbjarne commented May 9, 2019

[msbuild] Add reference to `System.Drawing.Common.dll` to XI projects. #6011

[msbuild] Add reference to `System.Drawing.Common.dll` to XI projects. #6011