[Xamarin.Android.Build.Tests] Reworks tests to use msbuild. #912

dellis1972 · 2017-10-03T10:03:46Z

This commit reworks the tests to work on msbuild by default.
xbuild is deprecated, so we should ensure that our system works
100% on msbuild.

So allot of changes in the system here. The hightlights are

We have to NOT use BuildingInsideVisualStudio when building
under msbuild. Setting this property to true has a side effect
of telling msbuild .. "Dont bother to build ProjectReferences".
This is because under VS that is something the IDE takes care of.
It turns out that the bug we had to do with __NonExistantFile__
causing the csc task to run ALL the time was fixed.

All of the IsTargetSkipped checks have been unified to call
IsTargetSkipped method out BuildOutput. It has also been updated
to include more target "skipped" formats which xbuild and msbuild
supports.

There is also a difference in behaviour between xbuild and msbuild.
You may well have noticed that ALL the tests that have had the

"debug" -> "release"

changes (including the debugger attribute one) are around

$(DebugType) == "None"

Under xbuild when $(DebugType) == "None" and $(DebugSymbols) == "True"
we end up with $(DebugType) == "None" and $(DebugSymbols) == "True".
Which results in a debug runtime and apk.

However.... under msbuild when $(DebugType) == "None"
and $(DebugSymbols) == "True" we end up with $(DebugType) == "portable"
and $(DebugSymbols) == "False".. so we get a release runtime and apk.

This is because None is NOT a valid value for $(DebugType) in msbuild,
so it resets BOTH $(DebugType) and $(DebugSymbols). So we have a
difference of behaviour between the two systems.

So in this commit we check for $(DebugType) == "None" and change it
and $(DebugSymbols) to match the msbuild behaviour. This will probably
break some QA tests.. but this is expected and the tests should be
updated to reflect this.

jonpryor · 2017-10-17T01:39:19Z

src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs

 		public string AssemblyIdentityMapFile { get; set; }

-		public string CacheFile { get; set;} 
+		public string CacheFile { get; set;}


Whitespace change?

jonpryor · 2017-10-17T01:41:22Z

What's troubling is that in getting the tests to "use msbuild," what the tests are asserting is changing.

...yet they're still passing, both before and after this change.

I am confused. :-/

jonpryor · 2017-10-17T01:42:37Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.OSS.cs

 				/* optimize */         true ,
 				/* embedassebmlies */  true ,
-				/* expectedResult */   "debug",
+				/* expectedResult */   "release",


e.g. here we go from expecting "debug" to "release". How is this related to using msbuild? Why's this still pass under xbuild?

jonpryor · 2017-10-17T01:43:01Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.OSS.cs

 			new object[] { "", false, true, },
 			new object[] { "None", true, false, },
-			new object[] { "None", false, false, },
+			new object[] { "None", false, true, },


...and here...

Guess this hits ManifestTest.DebuggerAttribute():

if (!isRelease && debugType == "None" && !builder.RunningMSBuild) expected = false

That still seems odd. Why would the presence/absence of DebuggerAttribute depend on xbuild vs. msbuild?

jonpryor · 2017-10-17T01:50:55Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/BuildOutput.cs

 			return Builder.LastBuildOutput.Contains (string.Format ("Target {0} skipped due to ", target))
-				      || Builder.LastBuildOutput.Contains (string.Format ("Skipping target \"{0}\" because its outputs are up-to-date", target))
-				      || Builder.LastBuildOutput.Contains ($"Skipping target \"{target}\" because all output files are up-to-date");
+					|| Builder.LastBuildOutput.Contains (string.Format ("Skipping target \"{0}\" because it has no outputs.", target))


I don't think this needs to be addressed in this PR, but I have to wonder...

Builder.LastBuildOutput is a string which contains the msbuild /v:diag build output, which could be many megabytes in size, right? And this is doing possibly 5 linear searches against that output.

How does that perform?

Would...regular expressions be better?

var toMatch = new[]{ $"Skipping target \"{target}\" because it has no outputs.", $"Target \"{target}\" skipped, due to", $"Skipping target \"{target}\" because its outputs are up-to-date", $"target {target}, skipping", $"Skipping target \"{target}\" because all output files are up-to-date", }; var r = new Regex (string.Join ("|", toMatch)); var m = r.Match (Builder.LastBuildOutput); return m.Success;

jonpryor · 2017-10-17T01:52:08Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/Builder.cs

-					if (!string.IsNullOrEmpty (Environment.GetEnvironmentVariable ("USE_MSBUILD"))) {
-						RunningMSBuild = true;
+					RunningMSBuild = true;
+					if (!string.IsNullOrEmpty (Environment.GetEnvironmentVariable ("USE_XBUILD"))) {


Shouldn't we stick with $USE_MSBUILD for now, and when $USE_MSBUILD is unset or 0 we assume xbuild

Eventually we'll make msbuild the default everywhere, and then it might -- might -- make sense to use $USE_XBUILD instead of $USE_MSBUILD...but now?

dellis1972 · 2017-10-17T09:09:21Z

@jonpryor I was going to cover that in the commit message once this works (its broken again since the xabuild.exe updates).

The problem is a difference in behaviour between xbuild and msbuild. You may well have noticed that ALL the tests that have had the "debug" -> "release" changes (including the debugger attribute one) are around $(DebugType) == "None"

Under xbuild when $(DebugType) == "None" and $(DebugSymbols) == "True" we end up with $(DebugType) == "None" and $(DebugSymbols) == "True". Which results in a debug runtime and apk.

However.... under msbuild when $(DebugType) == "None" and $(DebugSymbols) == "True" we end up with $(DebugType) == "portable" and $(DebugSymbols) == "False".. so we get a release runtime and apk.

This is because None is NOT a valid value for $(DebugType) in msbuild, so it resets BOTH $(DebugType) and $(DebugSymbols). So we have a difference of behaviour between the two systems. I was thinking today we should hack our targets to force xbuild to work the same way. So we can get consistent test results without any of the hacks in the tests. But I'm not sure what the effects would be, its almost certain that some of QA's tests will fail.

This commit reworks the tests to work on msbuild by default. xbuild is deprecated, so we should ensure that our system works 100% on msbuild. So allot of changes in the system here. The hightlights are We have to NOT use `BuildingInsideVisualStudio` when building under `msbuild`. Setting this property to `true` has a side effect of telling `msbuild` .. "Dont bother to build ProjectReferences". This is because under VS that is something the IDE takes care of. It turns out that the bug we had to do with `__NonExistantFile__` causing the `csc` task to run ALL the time was fixed. All of the `IsTargetSkipped` checks have been unified to call `IsTargetSkipped` method out BuildOutput. It has also been updated to include more target "skipped" formats which xbuild and msbuild supports. There is also a difference in behaviour between xbuild and msbuild. You may well have noticed that ALL the tests that have had the "debug" -> "release" changes (including the debugger attribute one) are around $(DebugType) == "None" Under xbuild when $(DebugType) == "None" and $(DebugSymbols) == "True" we end up with $(DebugType) == "None" and $(DebugSymbols) == "True". Which results in a debug runtime and apk. However.... under msbuild when $(DebugType) == "None" and $(DebugSymbols) == "True" we end up with $(DebugType) == "portable" and $(DebugSymbols) == "False".. so we get a release runtime and apk. This is because None is NOT a valid value for $(DebugType) in msbuild, so it resets BOTH $(DebugType) and $(DebugSymbols). So we have a difference of behaviour between the two systems. So in this commit we check for $(DebugType) == "None" and change it and $(DebugSymbols) to match the msbuild behaviour. This will probably break some QA tests.. but this is expected and the tests should be updated to reflect this.

This commit reworks the tests to pass on MSBuild. xbuild is deprecated, so we should ensure that our system works on MSBuild. Lots of changes. Highlights include: We have to NOT use `$(BuildingInsideVisualStudio)` when building under `msbuild`. Setting this property to `true` has a side effect of telling `msbuild` "Dont bother to build @(ProjectReferences)". This is because under VS that is something the IDE takes care of. It turns out that the bug we had to do with `__NonExistantFile__` causing the `csc` task to run ALL the time was fixed. All of the `IsTargetSkipped` checks have been unified to call the `BuildOutput.IsTargetSkipped()` method. It has also been updated to include more target "skipped" formats which xbuild and MSBuild generate. There is also a difference in behaviour between xbuild and MSBuild. You may well have noticed that ALL the tests that have had the "debug" -> "release" changes (including the debugger attribute one) are around $(DebugType) == "None" Under xbuild when `$(DebugType)` == "None" and `$(DebugSymbols)` == "True", we end up with `$(DebugType)` == "None" and `$(DebugSymbols)` == "True" (no changes). This results in a debug runtime and apk. However, under MSBuild when `$(DebugType)` == "None" and `$(DebugSymbols)` == "True" we end up with `$(DebugType)` == "portable" and `$(DebugSymbols)` == "False" (!). This results in a release runtime and apk. This is because `None` is NOT a valid value for `$(DebugType)` in MSBuild, so it resets BOTH `$(DebugType)` and `$(DebugSymbols)`. This results in difference of behaviour between the two systems. Update `Xamarin.Android.Common.targets` to check for this, and update `$(DebugType)` and `$(DebugSymbols)` so that xbuild behaves like MSBuild. This improves consistency (and our own sanity!). (This may break some QA tests, but this is now expected and the tests should be updated to reflect this.)

Fixes: dotnet/java-interop#857 Changes: dotnet/java-interop@087684a...0293360 * dotnet/java-interop@0293360a: [Xamarin.Android.Tools.Bytecode] NRT Support (dotnet#913) * dotnet/java-interop@e85564c1: [generator] Import NRT data from managed assemblies (dotnet#912) * dotnet/java-interop@c476cc39: [ci] Update to use VS2022 build agents (dotnet#914)

Fixes: dotnet/java-interop#857 Changes: dotnet/java-interop@087684a...0293360 * dotnet/java-interop@0293360a: [Xamarin.Android.Tools.Bytecode] NRT Support (#913) * dotnet/java-interop@e85564c1: [generator] Import NRT data from managed assemblies (#912) * dotnet/java-interop@c476cc39: [ci] Update to use VS2022 build agents (#914)

dellis1972 added the do-not-merge PR should not be merged. label Oct 3, 2017

dnfclas added the cla-already-signed label Oct 3, 2017

dellis1972 force-pushed the switchtomsbuild branch 3 times, most recently from 2efd5ba to beb6cbd Compare October 11, 2017 12:21

dellis1972 force-pushed the switchtomsbuild branch from 291a049 to 9f9d2ac Compare October 16, 2017 10:06

dellis1972 removed the do-not-merge PR should not be merged. label Oct 16, 2017

jonpryor reviewed Oct 17, 2017

View reviewed changes

dellis1972 added the merge-conflict label Oct 17, 2017

dellis1972 force-pushed the switchtomsbuild branch 2 times, most recently from 430cde4 to 8ea920b Compare October 18, 2017 09:57

dellis1972 removed the merge-conflict label Oct 18, 2017

dellis1972 force-pushed the switchtomsbuild branch from c0e66c0 to 5e83e5d Compare October 18, 2017 16:17

dellis1972 changed the title ~~[Xamarin.Android.Build.Tests] Reworks tests to use msbuild. [WIP]~~ [Xamarin.Android.Build.Tests] Reworks tests to use msbuild. Oct 18, 2017

jonpryor merged commit 8643ded into dotnet:master Oct 18, 2017

jonpryor mentioned this pull request Nov 16, 2021

Bump to xamarin/Java.Interop/main@0293360a #6497

Merged

github-actions bot locked and limited conversation to collaborators Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Xamarin.Android.Build.Tests] Reworks tests to use msbuild. #912

[Xamarin.Android.Build.Tests] Reworks tests to use msbuild. #912

Uh oh!

dellis1972 commented Oct 3, 2017 •

edited

Loading

Uh oh!

jonpryor Oct 17, 2017

Uh oh!

jonpryor commented Oct 17, 2017

Uh oh!

jonpryor Oct 17, 2017

Uh oh!

jonpryor Oct 17, 2017

Uh oh!

jonpryor Oct 17, 2017

Uh oh!

jonpryor Oct 17, 2017

Uh oh!

jonpryor Oct 17, 2017

Uh oh!

dellis1972 commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Xamarin.Android.Build.Tests] Reworks tests to use msbuild. #912

[Xamarin.Android.Build.Tests] Reworks tests to use msbuild. #912

Uh oh!

Conversation

dellis1972 commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Oct 17, 2017

Uh oh!

jonpryor Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

dellis1972 commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dellis1972 commented Oct 3, 2017 •

edited

Loading