[Xamarin.Android.Build.Tasks] Fix an issue where Facade assemblies are deployed #337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonpryor
reviewed
Dec 15, 2016
|
|
||
| foreach (var dir in ReferenceAssembliesDirectory.Split (new char[] { ';' }, StringSplitOptions.RemoveEmptyEntries)) | ||
| resolver.SearchDirectories.Add (dir); | ||
|
|
||
| var assemblies = new HashSet<string> (); | ||
| var assemblies = new HashSet<ITaskItem> (); |
There was a problem hiding this comment.
Is this safe or desirable?
The TaskItem documentation states that GetHashCode() and Equals(Object) are "(Inherited from Object.)". The source code for TaskItem likewise doesn't override GetHashCode() or Equals() or implement IEquatable<T>.
Consequently, this should thus be using referential equality, which means the following will store two values for the same path, not one:
var assemblies = new HashSet<ITaskItem> {
new TaskItem ("foo/bar"),
new TaskItem ("foo/bar"),
};
// assemblies.Count == 2
This almost certainly isn't what we want. Perhaps we should use the HashSet<T>(IEqualityComparer<T>) constructor?
dellis1972
force-pushed
the
fixassemblies
branch
2 times, most recently
from
c8ac02c
to
531ad2b
Compare
|
This is really annoying, it doesn't fail locally :/ |
dellis1972
force-pushed
the
fixassemblies
branch
from
531ad2b
to
849377d
Compare
dellis1972
force-pushed
the
fixassemblies
branch
5 times, most recently
from
ef7a3c4
to
3fb990b
Compare
…e deployed A change "somewhere" has recently causes this issue. The problem is in Release mode we now include the Facade assemblies in the package. Which is something we definately should NOT be doing. That said we do need the Facades in order for Linking and Aot to work since Cecil needs to be able to load them. So what this commit does is make sure the ResolveAssemblies task adds metadata for the Facades. We can then use that in the BuildApk Task to filter out the assemblies we do not want to ship.
dellis1972
force-pushed
the
fixassemblies
branch
from
3fb990b
to
e19cb85
Compare
|
Should this contain a test? |
|
Tests will be included in a separate PR. |
jonpryor
added a commit
that referenced
this pull request
Jan 10, 2017
…#357) Reverts #337 Commit a7beb70 broke unit test execution. [Jenkins build #190][b190] ran 128 tests, while [Jenkins build #191][b190] executed *6*. Build #191 only contains two commits, and commit bf1bc93 -- [PR #354][pr354] -- doesn't look like a plausible source of error. The cause of the "broken unit test execution" is that the on-device unit tests are exiting [with an error][x191]: Task "RunInstrumentationTests" .... INSTRUMENTATION_RESULT: shortMsg=android.runtime.JavaProxyThrowable INSTRUMENTATION_RESULT: longMsg=android.runtime.JavaProxyThrowable: System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime.Serialization, Version=2.0.5.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e' or one of its dependencies. ... xamarin-android/build-tools/scripts/UnitTestApks.targets: warning : No `nunit2-results-path` bundle value found in command output! Cannot find NUnit2 XML output! Commit a7beb70 stopped deploying facade assemblies, including `System.Runtime.Serialization`, which is why it can't be found. Revert commit a7beb70 so that on-device unit tests execute. [b190]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/190/testReport/ [b191]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/191/testReport/ [x191]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/191/consoleText [pr354]: #354
|
This PR prevented unit tests from executing on-device, because (shock!) the unit tests required the facade assemblies on-device: |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A change "somewhere" has recently causes this issue. The problem is
in Release mode we now include the Facade assemblies in the package.
Which is something we definately should NOT be doing.
That said we do need the Facades in order for Linking and Aot to
work since Cecil needs to be able to load them.
So what this commit does is make sure the ResolveAssemblies task
adds metadata for the Facades. We can then use that in the BuildApk
Task to filter out the assemblies we do not want to ship.