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 unmangling of satellite assembly names #9533

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

grendello
Copy link
Contributor

Fixes: #9532
Context: 86260ed

All the assemblies are wrapped in a valid ELF shared library image and
placed in the lib/{ABI} directories inside the APK/AAB archive. Since
those directories don't support subdirectories, we need to encode
satellite assembly culture in a way that doesn't use the / directory
separator char. This encoding, as originally implemented, unfortunately
used the - character which made it ambiguous with culture names that
consist of two parts (e.g. de-DE), since the unmangling process would
look for the first occurrence of - to replace it with /, which would
form invalid assembly names such as de/DE-MyAssembly.resources.dll
instead of the correct de-DE/MyAssembly.resources.dll. This would,
eventually, lead to a mismatch when looking for satellite assembly for
that specific culture.

Fix it by changing the - character to _ so that mangled assembly
name looks like lib_de-DE_MyAssembly.resources.dll.so and we can
unambiguously decode it to the correct de-DE/MyAssembly.resources.dll
name.

@grendello grendello force-pushed the dev/grendel/satellite-assembly-name-fix branch from 9eef8f7 to c3ab3f7 Compare November 21, 2024 10:14
@grendello
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Fixes: #9532
Context: 86260ed

All the assemblies are wrapped in a valid ELF shared library image and
placed in the `lib/{ABI}` directories inside the APK/AAB archive.  Since
those directories don't support subdirectories, we need to encode
satellite assembly culture in a way that doesn't use the `/` directory
separator char.  This encoding, as originally implemented, unfortunately
used the `-` character which made it ambiguous with culture names that
consist of two parts (e.g. `de-DE`), since the unmangling process would
look for the first occurrence of `-` to replace it with `/`, which would
form invalid assembly names such as `de/DE-MyAssembly.resources.dll`
instead of the correct `de-DE/MyAssembly.resources.dll`.  This would,
eventually, lead to a mismatch when looking for satellite assembly for
that specific culture.

Fix it by changing the `-` character to `_` so that mangled assembly
name looks like `lib_de-DE_MyAssembly.resources.dll.so` and we can
unambiguously decode it to the correct `de-DE/MyAssembly.resources.dll`
name.
Add a test for a compound culture satellite assembly.
@grendello grendello force-pushed the dev/grendel/satellite-assembly-name-fix branch from c3ab3f7 to 77d791f Compare November 22, 2024 11:19
@jonpryor
Copy link
Member

Fixes: https://github.com/dotnet/android/issues/9532

Context: https://github.com/dotnet/android/pull/9410
Context: 86260ed36dfe1a90c8ed6a2bb1cd0607d637f403
Context: c92702619f5fabcff0ed88e09160baf9edd70f41

In Debug configuration builds, `$(EmbedAssembliesIntoApk)`=false by
default.  This enables Fast Deployment in commercial/non-OSS builds.

When `$(EmbedAssembliesIntoApk)`=true, there are two separate ways to
embed assemblies into the `.apk`:

  * Assembly Stores (c9270261), which is a "single" (-ish) file that
    contains multiple assemblies, enabled by setting
    `$(AndroidUseAssemblyStore)`=true.

    This is the default behavior for Release configuration builds.

  * One file per assembly (86260ed3).

    This is the default behavior for Debug configuration builds when
    `$(EmbedAssembliesIntoApk)`=true.

Aside: dotnet/android#9410 is an attempt to *remove* support for the
"one file per assembly" packaging strategy, which will *not* be
applied to release/9.0.1xx.

When using the "one file per assembly" strategy, all the assemblies
are wrapped in a valid ELF shared library image and placed in the
`lib/{ABI}` directories inside the APK/AAB archive.  Since those
directories don't support subdirectories, we need to encode satellite
assembly culture in a way that doesn't use the `/` directory
separator char.

This encoding, as originally implemented, unfortunately used the `-`
character which made it ambiguous with culture names that consist of
two parts, e.g. `de-DE`, since the unmangling process would look for
the first occurrence of `-` to replace it with `/`, which would form
invalid assembly names such as `de/DE-MyAssembly.resources.dll`
instead of the correct `de-DE/MyAssembly.resources.dll`.  This would,
eventually, lead to a mismatch when looking for satellite assembly for
that specific culture.

Fix it by changing the encoding for `/` from `-` to `_`, so that the
mangled assembly name looks like
`lib_de-DE_MyAssembly.resources.dll.so` and we can unambiguously
decode it to the correct `de-DE/MyAssembly.resources.dll` name.

@jonpryor jonpryor merged commit 6394773 into main Nov 22, 2024
58 checks passed
@jonpryor jonpryor deleted the dev/grendel/satellite-assembly-name-fix branch November 22, 2024 17:30
jonpryor pushed a commit that referenced this pull request Nov 25, 2024
Fixes: #9532

Context: #9410
Context: 86260ed
Context: c927026

In Debug configuration builds, `$(EmbedAssembliesIntoApk)`=false by
default.  This enables Fast Deployment in commercial/non-OSS builds.

When `$(EmbedAssembliesIntoApk)`=true, there are two separate ways to
embed assemblies into the `.apk`:

  * Assembly Stores (c927026), which is a "single" (-ish) file that
    contains multiple assemblies, enabled by setting
    `$(AndroidUseAssemblyStore)`=true.

    This is the default behavior for Release configuration builds.

  * One file per assembly (86260ed).

    This is the default behavior for Debug configuration builds when
    `$(EmbedAssembliesIntoApk)`=true.

Aside: #9410 is an attempt to *remove* support for the
"one file per assembly" packaging strategy, which will *not* be
applied to release/9.0.1xx.

When using the "one file per assembly" strategy, all the assemblies
are wrapped in a valid ELF shared library image and placed in the
`lib/{ABI}` directories inside the APK/AAB archive.  Since those
directories don't support subdirectories, we need to encode satellite
assembly culture in a way that doesn't use the `/` directory
separator char.

This encoding, as originally implemented, unfortunately used the `-`
character which made it ambiguous with culture names that consist of
two parts, e.g. `de-DE`, since the unmangling process would look for
the first occurrence of `-` to replace it with `/`, which would form
invalid assembly names such as `de/DE-MyAssembly.resources.dll`
instead of the correct `de-DE/MyAssembly.resources.dll`.  This would,
eventually, lead to a mismatch when looking for satellite assembly for
that specific culture.

Fix it by changing the encoding for `/` from `-` to `_`, so that the
mangled assembly name looks like
`lib_de-DE_MyAssembly.resources.dll.so` and we can unambiguously
decode it to the correct `de-DE/MyAssembly.resources.dll` name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Satellite assemblies for other locales than the default are not deployed when EmbedAssembliesIntoApk = true
3 participants