[dotnet] Test integration by stephen-hawley · Pull Request #18543 · dotnet/macios (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation138 Commits32 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

Fixed an issue where the C# class name was coming in with , AssemblyName tacked on the end.
Fixed an issue where a class that had an entry in the map didn't have a class_ptr which was causing an NRE.
Fixed a predicate for deciding when to mark the assembly for save.

Note - for an as yet undetermined reason, the linker is not picking up that I'm marking the assembly for save, which is why the true test cases are commented out.

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

This comment has been minimized.

Comment on lines 124 to 136

switch (platform) {
case ApplePlatform.iOS:
return "Microsoft.iOS.dll";
case ApplePlatform.TVOS:
return "Microsoft.tvOS.dll";
case ApplePlatform.WatchOS:
return "Microsoft.WatchOS.dll";
case ApplePlatform.MacOSX:
return "Microsoft.macOS.dll";
case ApplePlatform.MacCatalyst:
return "Microsoft.MacCatalyst.dll";
default:
throw new NotImplementedException ($"Unknown platform: {platform}");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pattern matching could make the mapping a bit cleaner

switch (platform) {
case ApplePlatform.iOS:
return "Microsoft.iOS.dll";
case ApplePlatform.TVOS:
return "Microsoft.tvOS.dll";
case ApplePlatform.WatchOS:
return "Microsoft.WatchOS.dll";
case ApplePlatform.MacOSX:
return "Microsoft.macOS.dll";
case ApplePlatform.MacCatalyst:
return "Microsoft.MacCatalyst.dll";
default:
throw new NotImplementedException ($"Unknown platform: {platform}");
platform switch {
ApplePlatform.iOS => "Microsoft.iOS.dll",
ApplePlatform.TVOS => "Microsoft.tvOS.dll",
ApplePlatform.WatchOS => "Microsoft.WatchOS.dll",
ApplePlatform.MacOSX => "Microsoft.macOS.dll",
ApplePlatform.MacCatalyst => "Microsoft.MacCatalyst.dll",
_ => throw new NotImplementedException ($"Unknown platform: {platform}"),
}

Comment on lines 232 to 233

for (int i = 0; i < body.Instructions.Count; i++) {
var instr = body.Instructions [i];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the index isn't being used for anything other than accessing the instruction, so maybe a foreach would be better here?

for (int i = 0; i < body.Instructions.Count; i++) {
var instr = body.Instructions [i];
foreach (var instr in body.Instructions) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haritha-mohan This will create an invalid operation exception. Body.Instructions is a collection type and later on we modify the contents of the collection which invalidates the enumerator.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephen-hawley ah fair enough, the source code from cecil shows how the replace method does modify the collection so the index is necessary. thanks for clarifying!

@ivanpovazan -

@stephen-hawley regarding:
Not sure what would be the best approach here, whether the optimization should be moved out into a separate custom linker step that happens before the OutputStep, or if it can become part of an existing custom step which has the BeforeStep="OutputStep" condition.

The problem with moving this particular optimization is that it depends on the output of the RegistrarStep. It can't precede the RegistrarStep without doing all the work that the registrar does. We could try moving the entire registrar step to happen before the output step, but I don't know what the consequences of that are. However, we probably don't want the StaticRegistrar run until the link has been completed. This seems like a chicken/egg problem here.
Can I write into obj/linked safely and be confident that those changes will get picked up by the rest of the process?

Can I write into obj/linked safely and be confident that those changes will get picked up by the rest of the process?
The answer to this is yes, you can write to the assembly, but no, it does not end up in the output.

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

This looks legitimate:

Xamarin.Tests.RegistrarTest.ClassRewriterTest(TVOS,False): There are fields in classHandles - rewriter was called when it should have done nothing.
Expected: True
But was: False

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

This comment has been minimized.

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13.0) passed.

Pipeline on Agent
Hash: [PR build]

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: [PR build]

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected) NET (empty diffs)
~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes) .NET (No breaking changes)
iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist Legacy Xamarin (stable) vs .NET
iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 6aa32607a8db9fe43a816240220c9fdd9603820a [PR build]