Change all null checking expressions to use 'is null' and 'is not null'
instead of '== null' and '!= null'.
This was mostly done with sed, so code can probably be improved in many
other ways with manual inspection, but that will come over time.
Also add code to the autoformat script to automatically fix these issues in the future.
TL&DR
* re-apply the fix to cache.cs from https://github.com/xamarin/xamarin-macios/pull/7544
* which was reverted in https://github.com/xamarin/xamarin-macios/pull/7589
* since it regressed mscorlib/sim testing in xharness (for other reasons)
* Final part to fix https://github.com/xamarin/xamarin-macios/issues/7514
This was the ~night~ day before christmas... amd a tough nut to crack!
Thanksfully we had a good test case (inside #7514) and then xharness
regressed one test in consistent, reproducible manner.
xharness builds mscorlib tests twice (32 and 64 bits) even if it's a
fat application (could be reused). That should not be a huge problem
since the 2nd build should be identical and the cache should be (re)used.
An earlier attempt fixed this (comparison was true for the wrong
reasons [1]) but the fix did not end up with the same arguments !?! and
was reverted.
This is the diff between the first and second builds:
```diff
--- /Users/poupou/a.txt 2019-12-23 09:55:01.000000000 -0500
+++ /Users/poupou/b.txt 2019-12-23 09:55:01.000000000 -0500
@@ -182,5 +182,5 @@
-r=/Users/poupou/git/xamarin/xamarin-macios/builds/downloads/ios-release-Darwin-8f396bbb408b5758fccb8602030b9fa5293ce718/ios-bcl/monotouch/tests/Xunit.NetCore.Extensions.dll \
' --target-framework=Xamarin.iOS,v1.0' \
--root-assembly=/Users/poupou/git/xamarin/xamarin-macios/tests/xharness/tmp-test-dir/mscorlib/bin/mscorlib/iPhoneSimulator/Debug-unified/com.xamarin.bcltests.mscorlib.exe \
- ' -v -v -v -v' \
+ ' -v -v' \
@/Users/poupou/git/xamarin/xamarin-macios/tests/xharness/tmp-test-dir/mscorlib/obj/iPhoneSimulator/Debug/response-file.rsp \
```
Since they are not identical the cache is invalidated (which is normal,
cache-wise) and produce an output app that is incorrect (and crash
32bits).
Now there is code to ignore verbosity options (both `-v` and -q`) since
they will not affect what `mtouch` generates. However this was broken
because mtouch's response-file parser is quite basic and stricter the the
specification
spec: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/response-file-compiler-option
issue: https://github.com/xamarin/xamarin-macios/issues/7644
That failed in two different ways
1. note the extra space before the first `-v` in the diff (before the `'`
quote). That skipped the line.
2. there are multiple `-v` in the same line, again that make the
filtering skip the line.
*Unknowns*
It's too close to xmas/vacation so I might not find the reasons/issues
for the following, unanswered questions...
1. Why is the re-build app bundle failing at runtime when p/invoking ?
Something is not regenerated (symbol maps?) ?
2. Why xharness 2nd build has more verbosity than the first one (likely
harmless) ?
[1] the original cache.cs issue (prequel)
issue w/test case: https://github.com/xamarin/xamarin-macios/issues/7514
first attempt: https://github.com/xamarin/xamarin-macios/pull/7544
While incorrect the first attempt to fix `cache.cs` was a logical, if
not entirely complete, fix. Without it this is what we _currently_ cache:
```
/Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/lib/mtouch/mtouch.exe \
```
and that does not include any of mtouch's arguments, that can change
between executions and (should) invalidate the cache.
In this case it means the cache is used (no difference) but this does
**not** parse the content of the **response file** which is obviously
wrong (and we do have code to process it).
On the original issue's test case this is what makes the difference
between using the same *old* nuget assembly after an update (and fail)
by itself and also because the updated framework was not copied (due
to the 2nd part of the bug report wrt `copyfile`).
OTOH re-using (incorrectly) the cache is what makes xharness's mscorlib
unit tests works right now :(
The nice, repeatable test case from #7514 pointed out two issues
1. `cache.cs` ignored some changes
It looks like something changed (at some point) and the first _ignored_
line was the `@x.rsp` our response file - which should not be ignored.
This solved the build issue where updating the nuget should have
triggered a rebuild because
> /Users/poupou/.nuget/packages/skiasharp/1.68.0/lib/Xamarin.iOS/SkiaSharp.dll
and
> /Users/poupou/.nuget/packages/skiasharp/1.68.1/lib/Xamarin.iOS/SkiaSharp.dll
are different assemblies (but the same response file).
2. `copyfile` could fail silently
Copying the framework could fail (error 260) and the failure was never
reported so the build succeeded - but without updating (completely) the
framework.
The exact reason it fails is unknown :( but we can recover from it by
deleting the target and copying (everything) back to the expected
(target) location.
Build logs will now indicate when this fails and will try to recover
before reporting a build error. Best case it works :) worse case we'll
be aware something is wrong (which is better than ignoring)
ref: https://github.com/xamarin/xamarin-macios/issues/7514
* Do not call `Marshal.GetLastWin32Error` instead the callback
as the `SetLastError` logic has yet to be executed so we get a bogus
`260` value...
Instead we call it after the `copyfile` call returns but, at this stage,
the callback (i.e. **us**) signaled an error so what we get back (`17`)
is not very helpful - as we aborted (Quit) the logic when copying a file
that existed.
```c
#define EEXIST 17 /* File exists */
```
from Console logs
```
default 14:23:54.771292-0500 mono64 Cannot make directory /Users/poupou/Projects/gh7514/gh7514/bin/iPhoneSimulator/Debug/gh7514.app/Frameworks/libSkiaSharp.framework: File exists
default 14:23:54.771620-0500 mono64 Cannot make directory /Users/poupou/Projects/gh7514/gh7514/bin/iPhoneSimulator/Debug/gh7514.app/Frameworks/libSkiaSharp.framework/_CodeSignature: File exists
default 14:23:54.771850-0500 mono64 open on /Users/poupou/Projects/gh7514/gh7514/bin/iPhoneSimulator/Debug/gh7514.app/Frameworks/libSkiaSharp.framework/_CodeSignature/CodeResources: File exists
```
`copyfile.c` source code (might not be the latest) can be seen from
https://opensource.apple.com/source/copyfile/copyfile-42/copyfile.c
```c
if (mkdir(s->dst, mode) == -1) {
if (errno != EEXIST || (s->flags & COPYFILE_EXCL)) {
copyfile_warn("Cannot make directory %s", s->dst);
```
so `mkdir` fails - but not because of `EEXIST` and from `copyfile.h`
we see that `EXCL` exists but it's not using (or even defined) in our
bindings.
```c
#define COPYFILE_EXCL (1<<17) /* fail if destination exists */
```
So sadly the `Err` condition (inside the callback) does not give us more
detail about the error itself.
* Implement a different escaping/quoting algorithm for arguments to System.Diagnostics.Process.
mono changed how quotes should be escaped when passed to
System.Diagnostic.Process, so we need to change accordingly.
The main difference is that single quotes don't have to be escaped anymore.
This solves problems like this:
System.ComponentModel.Win32Exception : ApplicationName='nuget', CommandLine='restore '/Users/vsts/agent/2.158.0/work/1/s/tests/sampletester/bin/Debug/repositories/ios-samples/WorkingWithTables/Part 3 - Customizing a Table\'s appearance/3 - CellCustomTable/CellCustomTable.sln' -Verbosity detailed -SolutionDir '/Users/vsts/agent/2.158.0/work/1/s/tests/sampletester/bin/Debug/repositories/ios-samples/WorkingWithTables/Part 3 - Customizing a Table\'s appearance/3 - CellCustomTable'', CurrentDirectory='/Users/vsts/agent/2.158.0/work/1/s/tests/sampletester/bin/Debug/repositories', Native error= Cannot find the specified file
at System.Diagnostics.Process.StartWithCreateProcess (System.Diagnostics.ProcessStartInfo startInfo) [0x0029f] in /Users/builder/jenkins/workspace/build-package-osx-mono/2019-08/external/bockbuild/builds/mono-x64/mcs/class/System/System.Diagnostics/Process.cs:778
ref: https://github.com/mono/mono/pull/15047
* Rework process arguments to pass arrays/lists around instead of quoted strings.
And then only convert to a string at the very end when we create the Process
instance.
In the future there will be a ProcessStartInfo.ArgumentList property we can
use to give the original array/list of arguments directly to the BCL so that
we can avoid quoting at all. These changes gets us almost all the way there
already (except that the ArgumentList property isn't available quite yet).
We also have to bump to target framework version v4.7.2 from v4.5 in several
places because of 'Array.Empty<T> ()' which is now used in more places.
* Parse linker flags from LinkWith attributes.
* [sampletester] Bump to v4.7.2 for Array.Empty<T> ().
* Fix typo.
* Rename GetVerbosity -> AddVerbosity.
* Remove unnecessary string interpolation.
* Remove unused variable.
* [mtouch] Simplify code a bit.
* Use implicitly typed arrays.
If both an extension and the container app (or multiple extensions) reference
the same binding assembly for a framework, then we'd error out with an
internal error when trying to copy the framework from both locations to the
container app.
So instead detect when we're trying to copy multiple identical (by comparing
the on-disk contents) frameworks, and only copy one of them.
We'll still show an error if the frameworks are different, but now a nice
MT1035 error with a proper error description instead of an internal error.
https://bugzilla.xamarin.com/show_bug.cgi?id=56635
This fixes a file sharing exception:
> MTOUCH: error MT1009: Could not copy the assembly '[...]/msbuild/tests/MyActionExtension/bin/iPhone/Debug/MyActionExtension.dll' to '[...]/msbuild/tests/MyTabbedApplication/obj/iPhone/Debug/mtouch-cache/32/Link/MyActionExtension.dll': Sharing violation on path [...]/msbuild/tests/MyActionExtension/bin/iPhone/Debug/MyActionExtension.pdb
Change cache invalidation so that if any app extension's cache is invalid,
then invalidate the cache for the container app and all other app extensions.
This is the safest option when we're sharing code.
Don't use the global command line arguments to determine input, because that's
not the input we use for app extensions anymore.
Instead explicitly pass the input arguments when creating the cache.
Since neither mtouch nor mmmp is mkbundled anymore, the installed binary is in
fact a shell script.
This means that it's quite useless to check if the shell script has been
modified; instead check if the executing assembly has been modified (which
works now that we're not mkbundled anymore).
So that there can be multiple caches in the same process (which we'll have
once mtouch can compile extensions and the container app in the same process).
Rolf Bjarne Kvinge
Sebastien Pouliot
Timothy Risi