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.

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

Fix assembly count when satellite assemblies are present #8790

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Mar 6, 2024

NOTE: should be backported to net8

Fixes: #8789

The old method of satellite assembly counting relied on the
RelativePath MSBuild item metadata, which appears to have gone
missing somewhere in .NET8+. Update the code to check for presence
of the following metadata, in order given, to determine assembly's
culture, if any:

  • Culture
  • RelativePath
  • DestinationSubDirectory

Failure to count satellite assemblies can, and sometimes will,
result in a segfault since we generate a number of native code
arrays based on the assembly count and the runtime assumes that
what the build told it is true.

Fixes: #8789

The old method of satellite assembly counting relied on the
`RelativePath` MSBuild item metadata, which appears to have gone
missing somewhere in .NET8+.  Update the code to check for presence
of the following metadata, in order given, to determine assembly's
culture, if any:

  * `Culture`
  * `RelativePath`
  * `DestinationSubDirectory`

Failure to count satellite assemblies can, and sometimes will,
result in a segfault since we generate a number of native code
arrays based on the assembly count and the runtime assumes that
what the build told it is true.
* main:
  [build] Include MIT license in most NuGet packages (#8787)
  Bump to dotnet/installer@893b762b6e 9.0.100-preview.3.24153.2 (#8782)
// The best option
string? culture = assembly.GetMetadata ("Culture");
if (!String.IsNullOrEmpty (culture)) {
return TrimSlashes (culture);
Copy link
Member

@akoeplinger akoeplinger Mar 7, 2024

Choose a reason for hiding this comment

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

nit: it'd be pretty weird if Culture contained any slashes, might be ok to return it unmodified nevermind I just saw a binlog that had a Culture with a slash :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I learned the hard way... :)

}

// ...slightly worse
culture = assembly.GetMetadata ("RelativePath");
Copy link
Member

Choose a reason for hiding this comment

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

So I checked the history and there was some discussion around RelativePath here: #7823 (review)

Not sure how that would've been set for satellite assemblies though, I can't find where it'd be set.

I'm only seeing Culture (for satellite assemblies in the current project) or DestinationSubDirectory (for satellites from referenced projects) being set.

Instead of checking every assembly can you only look at the satellite assemblies? you're already passing those to the GeneratePackageManagerJava task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we possibly should do that, but not until after #8478 is merged (it changes a lot in this area, and all the optimizations and tweaks to the code should be done only after the merge)

* main:
  Bump to xamarin/Java.Interop/main@a7e09b7 (#8793)
@jonpryor
Copy link
Member

Is there any way to add a unit test for this?

@grendello
Copy link
Contributor Author

grendello commented Mar 11, 2024

Is there any way to add a unit test for this?

We have tests that check the presence in the archive after build, but we have no on-device tests which actually use satellite assemblies it seems. I think it would be good to add a test, but I'd like to do it in a separate PR, which will implement better build-time handling of discrepancies.

@dellis1972
Copy link
Contributor

Is there any way to add a unit test for this?

We have tests that check the presence in the archive after build, but we have no on-device tests which actually use satellite assemblies it seems. I think it would be good to add a test, but I'd like to do it in a separate PR, which will implement better build-time handling of discrepancies.

The Localisation tests should use Satallite assemblies

@grendello
Copy link
Contributor Author

Is there any way to add a unit test for this?

We have tests that check the presence in the archive after build, but we have no on-device tests which actually use satellite assemblies it seems. I think it would be good to add a test, but I'd like to do it in a separate PR, which will implement better build-time handling of discrepancies.

The Localisation tests should use Satallite assemblies

Hm, interesting. They should have failed without this fix... Maybe we were just lucky with them, as this error may not happen if the satellite assembly hashes are in the upper part of the sorted array of hashes we generate at build time.

@dellis1972
Copy link
Contributor

Hm, interesting. They should have failed without this fix... Maybe we were just lucky with them, as this error may not happen if the satellite assembly hashes are in the upper part of the sorted array of hashes we generate at build time.

The tests add a bunch of resx files via https://github.com/xamarin/xamarin-android/blob/main/tests/MSBuildDeviceIntegration/Tests/LocalizationTests.cs#L49. That should generated satallite dlls.
Perhaps it needs updating to add the types of files that cause the problem. Is it satallites from library projects that cause the issue?

@grendello
Copy link
Contributor Author

@dellis1972 Unfortunately, it's a bit of a gamble when/if that happens. At build time we generate an array of xxHash hashes using several forms of assembly names as input (with and without extension) and then sort that array in ascending order. At run time, we search through that array with binary search. If the number of entries in that array is smaller than what we counted at build time, we may have a crash when binary search tries to access entries outside (above) that range. But if we're lucky, some assemblies might not be loaded/used during the time the test/app runs and if the used/loaded assemblies have their hashes fall within the actual hash array limits, we won't get the crash.

A correct test for this would be one where we're sure that all of the assemblies, including the satellite ones, are loaded on every run. The test would have to change the UI culture during single execution to make sure all satellite assemblies are loaded. I will work on a test like this, but only after the per-RID PR is merged, since it touches the way satellite assemblies are handled and I'd like to add a way to ensure, at build time, that we have the correct number of entries in each of the arrays. I don't want to add it to #8478 which is already big enough, but I also don't want to keep rebasing it on top of main changes too much.

@jonpryor
Copy link
Member

As #8478 has been merged, this PR (#8790) is no longer directly applicable to main.

Please update the PR to target the release/8.0.2xx branch.

* main:
  LEGO: Merge pull request 8818
  Bump to dotnet/installer@b40c44502d 9.0.100-preview.3.24165.20 (#8817)
  Bump com.android.tools:r8 from 8.2.47 to 8.3.37 (#8816)
  [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795)
  Localized file check-in by OneLocBuild Task (#8815)
  [Xamarin.Android.Build.Tasks] Make all assemblies RID-specific (#8478)
  Localized file check-in by OneLocBuild Task (#8813)
  [Xamarin.Android.Build.Tasks] %(AndroidAsset.AssetPack) Support (#8631)
  [runtime] Remove the last vestiges of desktop builds (#8810)
  [ci] Don't auto-retry APK test suites. (#8811)
  [Microsoft.Android.Templates] Update EN l10n template strings (#8808)
  Bump to xamarin/Java.Interop/main@651de42 (#8809)
  [Mono.Android] is now "trimming safe" (#8778)
  [Mono.Android] Fix missing enum issues that cause BG8800 warnings. (#8707)
  Bump external/Java.Interop from `3436a30` to `5bca8ad` (#8803)
  Bump to xamarin/monodroid@77124dc1 (#8804)
  Bump to dotnet/installer@e911f5c82c 9.0.100-preview.3.24161.2 (#8802)
  Bump to xamarin/Java.Interop/main@3436a30 (#8799)
  [templates] Remove redundant "template" from display name. (#8773)
@grendello
Copy link
Contributor Author

@jonpryor I think it might be good to merge it into main as well. I will need to open a separate PR for the net8 branch, as the changes don't apply cleanly

grendello added a commit that referenced this pull request Mar 20, 2024
Context: #8790

The old method of satellite assembly counting relied on the
`RelativePath` MSBuild item metadata, which appears to have gone
missing somewhere in .NET8+.  Update the code to check for presence
of the following metadata, in order given, to determine assembly's
culture, if any:

  * `Culture`
  * `RelativePath`
  * `DestinationSubDirectory`

Failure to count satellite assemblies can, and sometimes will,
result in a segfault since we generate a number of native code
arrays based on the assembly count and the runtime assumes that
what the build told it is true.
jonpryor pushed a commit that referenced this pull request Mar 22, 2024
Context: #8790

The old method of satellite assembly counting relied on the
`RelativePath` MSBuild item metadata, which appears to have gone
missing somewhere in .NET8+.  Update the code to check for presence
of the following metadata, in order given, to determine assembly's
culture, if any:

  * `Culture`
  * `RelativePath`
  * `DestinationSubDirectory`

Failure to count satellite assemblies can, and sometimes will,
result in a segfault since we generate a number of native code
arrays based on the assembly count and the runtime assumes that
what the build told it is true.
@jonpryor jonpryor merged commit 385091a into main Mar 22, 2024
48 checks passed
@jonpryor jonpryor deleted the dev/grendel/fix-satellite-assembly-count branch March 22, 2024 18:47
grendello added a commit that referenced this pull request Mar 27, 2024
* main:
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
grendello added a commit that referenced this pull request Mar 27, 2024
* main:
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
  LEGO: Merge pull request 8818
  Bump to dotnet/installer@b40c44502d 9.0.100-preview.3.24165.20 (#8817)
  Bump com.android.tools:r8 from 8.2.47 to 8.3.37 (#8816)
  [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795)
  Localized file check-in by OneLocBuild Task (#8815)
  [Xamarin.Android.Build.Tasks] Make all assemblies RID-specific (#8478)
  Localized file check-in by OneLocBuild Task (#8813)
grendello added a commit that referenced this pull request Apr 3, 2024
* main:
  Bump to dotnet/installer@dc43d363d2 9.0.100-preview.4.24175.5 (#8828)
  [Xamarin.Android.Build.Tasks] Update to newer ILRepack which supports debug files. (#8839)
  Bump 'NuGet.*' and 'Newtonsoft.Json' NuGet versions. (#8825)
  Localized file check-in by OneLocBuild Task (#8844)
  [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
  LEGO: Merge pull request 8837
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Startup crash with Release configuration on device
4 participants