Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add UseInterpreter as part of the check for DynamicCodeSupport #20912

Merged
merged 2 commits into from
Jul 19, 2024
Merged

Add UseInterpreter as part of the check for DynamicCodeSupport #20912

merged 2 commits into from
Jul 19, 2024

Conversation

mcumming
Copy link
Contributor

fixes: dotnet/maui#23577

Cause:

MtouchInterpreter is set as

xamarin-macios/msbuild/Xamarin.Shared/Xamarin.Shared.props

Line 308 in d1ec7a7

<MtouchInterpreter Condition="'$(MtouchInterpreter)' == '' And '$(UseInterpreter)' == 'True'">all</MtouchInterpreter>

DynamicCodeSupport is set by

xamarin-macios/dotnet/targets/Xamarin.Shared.Sdk.targets

Line 146 in d1ec7a7

<DynamicCodeSupport Condition="'$(DynamicCodeSupport)' == '' And '$(MtouchInterpreter)' == '' And ('$(_PlatformName)' == 'iOS' Or '$(_PlatformName)' == 'tvOS' Or '$(_PlatformName)' == 'MacCatalyst')">false</DynamicCodeSupport>

Xamarin.Shared.Sdk.targets is imported before Xamarin.Shared.props as shown below (courtesy of @ivanpovazan)

so unless the value of MtouchInterpreter is set in the project, it's value will be empty when the DynamicCodeSupport property is evaluated.

Resolution:

To minimize the impact of this change, until it can be investigated more fully, the value of MtouchInterpreter is evaluated as

xamarin-macios/msbuild/Xamarin.Shared/Xamarin.Shared.props

Line 308 in d1ec7a7

<MtouchInterpreter Condition="'$(MtouchInterpreter)' == '' And '$(UseInterpreter)' == 'True'">all</MtouchInterpreter>

So adding ( '$(MtouchInterpreter)' == '' And '$(UseInterpreter)' == 'false' ) to the definition of DynamicCodeSupport to match the MtouchInterpreter definition.

A further fix might be to either:

  1. Reorder the imports so that the props are included before the targets, although the ramifications of that change could be significant
  2. Move the definition of DynamicCodeSupport to Xamarin.Shared.props. This at first glance seems to be less significant than 1) but would need review and testing.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@mcumming @ivanpovazan
Update UseInterpreter condition to account for default of ''
419b2b8 
Co-authored-by: Ivan Povazan <55002338+ivanpovazan@users.noreply.github.com>
@ivanpovazan
Copy link
Contributor

ivanpovazan commented Jul 18, 2024 •
edited
Loading

It is worth noting that the cause of regression are these two changes: a72bb30 and b1fa068 /cc: @rolfbjarne

Additionally, those two changes regressed MAUI maccatalyst/ios debug builds (although I am not aware if anyone reported this yet) because MAUI forces UseInterpreter=true in their targets:
https://github.com/dotnet/maui/blob/09fc58c188827af65b33aa25e08677831dd0bf0a/src/Controls/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.Common.targets#L6
Which is also imported before MtouchInterpreter evaluation, making the DynamicCodeSupport have the wrong value set even in this case.

The good thing is that this PR also fixes that problem, but I wanted to draw attention to it so it is considered in any future work/improvements.

ivanpovazan
ivanpovazan approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ivanpovazan ivanpovazan mentioned this pull request Jul 18, 2024
Closed
@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [CI Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

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

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Big Sur (11) passed 💻

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

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ 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)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

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

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 168 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 6 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ install-source: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 7 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac-binding-project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 6 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (watchOS): All 4 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 419b2b86c8fadec67946014e6f8b1c79112ba3bf [PR build]

@mcumming mcumming marked this pull request as ready for review July 18, 2024 19:11
@dalexsoto
Copy link
Member

/sudo backport release/8.0.1xx-xcode15.4

@dalexsoto
Copy link
Member

/sudo backport release/8.0.1xx-xcode15.1

@mcumming
Copy link
Contributor Author

Locally tested:
image

dalexsoto added a commit that referenced this pull request Jul 19, 2024
@dalexsoto
[release/8.0.1xx-xcode15.4] Add UseInterpreter as part of the check f…
1c80797 
…or DynamicCodeSupport

Manual backport of #20912
@dalexsoto dalexsoto mentioned this pull request Jul 19, 2024
Merged
dalexsoto added a commit that referenced this pull request Jul 19, 2024
@dalexsoto
[release/8.0.1xx-xcode15.1] Add UseInterpreter as part of the check f…
631cbd2 
…or DynamicCodeSupport

Manual backport of #20912
@dalexsoto dalexsoto mentioned this pull request Jul 19, 2024
Merged
@dalexsoto dalexsoto merged commit 261a603 into main Jul 19, 2024
28 checks passed
@dalexsoto dalexsoto deleted the dev/mcumming/23577 branch July 19, 2024 19:18
dalexsoto added a commit that referenced this pull request Jul 19, 2024
@dalexsoto
[release/8.0.1xx-xcode15.4] Add UseInterpreter as part of the check f…
500f632 
…or DynamicCodeSupport (#20921)

Manual backport of #20912
dalexsoto added a commit that referenced this pull request Jul 20, 2024
@dalexsoto
[release/8.0.1xx-xcode15.1] Add UseInterpreter as part of the check f…
386b6b4 
…or DynamicCodeSupport (#20922)

Manual backport of #20912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haritha-mohan haritha-mohan haritha-mohan approved these changes

@ivanpovazan ivanpovazan ivanpovazan approved these changes

@dalexsoto dalexsoto dalexsoto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Workload 8.0.61 breaks Mono interpreter on MacCatalyst.
5 participants
@mcumming @vs-mobiletools-engineering-service2 @ivanpovazan @dalexsoto @haritha-mohan