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

Set TargetsLinuxGlibc to false on Android #109649

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Nov 8, 2024

It's not using glibc. One could argue that we should set TargetsLinuxBionic=true on Android but that'd go against how we used that name elsewhere.

Follow-up to #109526

It's not using glibc. One could argue that we should set TargetsLinuxBionic=true on Android but that'd go against how we used that name elsewhere.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 8, 2024
@akoeplinger akoeplinger added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 8, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented Nov 8, 2024

that'd go against how we used that name elsewhere.

Where would it break? I have spot checked a few places and it seems to be fine. It would allow us to simplify '$(TargetsAndroid)' == 'true' or '$(TargetsLinuxBionic)' == 'true' conditions that we have in number of places.

@jkoritzinsky
Copy link
Member

Yeah I'd assume that Android implies Linux Bionic and for the small number of cases where that differs, we'd check "Linux Bionic and not Android"

@akoeplinger
Copy link
Member Author

Example breakages:

  • we'd use the wrong runner script here:
    <RunScriptInputName Condition="'$(TargetsLinuxBionic)' == 'true' and '$(RunScriptWindowsCmd)' != 'true'">BionicRunnerTemplate.sh</RunScriptInputName>
  • we'd build the host subset here:
    <DefaultSubsets Condition="'$(TargetsMobile)' == 'true'">mono+libs+packs</DefaultSubsets>
    <DefaultSubsets Condition="'$(TargetsLinuxBionic)' == 'true'">mono+libs+host+packs</DefaultSubsets>
  • we'd use the wrong MonoSharedLibName here:
    <MonoSharedLibName Condition="('$(TargetsiOS)' == 'true' or '$(TargetstvOS)' == 'true' or '$(TargetsMacCatalyst)' == 'true' or '$(TargetsAndroid)' == 'true' or '$(TargetsBrowser)' == 'true') and '$(TargetsLinuxBionic)' != 'true'">$(MonoLibName)</MonoSharedLibName>

Those can be fixed of course and I don't really feel strongly one way or another but we need to be careful.

@akoeplinger
Copy link
Member Author

I think I prefer fixing TargetsLinuxBionic=true on Android instead, the number of places where it matters is indeed quite small. I'll update the PR (not today since it's almost weekend here 😄)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants