Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

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

Already on GitHub? # to your account

Fix last remaining test on Android #2457

Merged
merged 2 commits into from
Jul 4, 2023
Merged

Fix last remaining test on Android #2457

merged 2 commits into from
Jul 4, 2023

Conversation

finagolfin
Copy link
Contributor

@finagolfin finagolfin commented Jul 2, 2023

Motivation:

Get all tests passing on Android AArch64

Modifications:

  • Check if localhost is set for IPv6 in BootstrapTest/testClientBindWorksOnSocketsBoundToEitherIPv4OrIPv6Only, and use ip6-localhost if not

Result:

All tests pass natively on Android AArch64

The problem is that the localhost definitions in /etc/hosts on Android look like this, so I've long disabled this test on my Android CI:

> ag localhost /etc/hosts
127.0.0.1       localhost
::1             ip6-localhost

Whereas this test passed on linux because it defines localhost for both protocols:

> ag localhost /etc/hosts
127.0.0.1 localhost
::1 localhost ip6-localhost ip6-loopback

Binging it online, I see that Darwin doesn't appear to define ip6-localhost, so I kept that the same. I only tested this change natively on Android, so let's see if the CI catches anything on linux and macOS.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for this patch!

I worry about this code being a little brittle. Can I suggest that we instead do a DNS resolution on localhost and ip6-localhost to check whether the names are defined, and what they resolve to? That will make this code a lot more predictable, and work on systems regardless of how specifically they've handled /etc/hosts.

Motivation

Get all tests passing on Android AArch64

Modifications

- Check if localhost is set for IPv6 in BootstrapTest/testClientBindWorksOnSocketsBoundToEitherIPv4OrIPv6Only,
  and use ip6-localhost if not

Result

All tests pass natively on Android AArch64
@finagolfin
Copy link
Contributor Author

Made the DNS resolution change you asked for, let me know what you think.

@Lukasa Lukasa enabled auto-merge (squash) July 4, 2023 10:03
@Lukasa Lukasa merged commit 324bc65 into apple:main Jul 4, 2023
@Lukasa Lukasa added the semver/none No version bump required. label Jul 4, 2023
@finagolfin finagolfin deleted the droid branch July 4, 2023 12:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants