cargo icon indicating copy to clipboard operation
cargo copied to clipboard

fix: support IPv6-only network for cargo fix

Open weihanglo opened this issue 1 month ago • 6 comments

What does this PR try to resolve?

To coordinate diagnositcs between different cargo-as-rustc-proxies, cargo fix launches a diagnostc server on localhost. However, the TCP server was hard-coded with an IPv4 address 127.0.0.1, which didn't work with IPv6-only network.

This commit fixes the issue by attempting to bind both IPv4 and IPv6 localhost addessses.

Fixes #13906

How should we test and review this PR?

I have no idea how to test it. Maybe in Docker follow the IPv6 network setup guide, and disable IPv4 stack? Or create a VM that is completely IPv6-only.

I've manually tested it on a macbook. Use at your on risk:

$ ifconfig lo0
# make sure lo0 have both IPv4 and IPv6, and back up if needed
$ ifconfig lo0 inet delete
$ cargo clippy --fix # the original cargo should fail; this patch should pass
$ ifconfig lo0 inet 127.0.0.1 # restore

Additional information

weihanglo avatar May 12 '24 22:05 weihanglo

r? @ehuss

rustbot has assigned @ehuss. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot avatar May 12 '24 22:05 rustbot

Wow, nice!

$ export PATH="/Users/aryehh/Development/cargo/target/release:$PATH"
$ cargo --version
cargo 1.80.0 (ced58bfd6 2024-05-12)
$ cargo clippy --fix
    Updating crates.io index
  Downloaded rowan v0.15.15
  Downloaded countme v3.0.1
  Downloaded strum v0.26.2
  Downloaded rustc-hash v1.1.0
  Downloaded quote v1.0.36
  Downloaded memoffset v0.9.1
  Downloaded text-size v1.1.1
  Downloaded proc-macro2 v1.0.81
  Downloaded serde_derive v1.0.198
  Downloaded serde v1.0.198
  Downloaded rnix v0.11.0
  Downloaded syn v2.0.60
  Downloaded 12 crates (765.5 KB) in 0.25s
   Compiling proc-macro2 v1.0.81
   Compiling unicode-ident v1.0.12
   Compiling serde v1.0.198
   Compiling autocfg v1.2.0
   Compiling serde_json v1.0.116
    Checking countme v3.0.1
    Checking hashbrown v0.14.3
    Checking rustc-hash v1.1.0
    Checking itoa v1.0.11
    Checking ryu v1.0.17
    Checking strum v0.26.2
   Compiling memoffset v0.9.1
   Compiling quote v1.0.36
   Compiling syn v2.0.60
   Compiling serde_derive v1.0.198
    Checking text-size v1.1.1
    Checking rowan v0.15.15
    Checking rnix v0.11.0
    Checking rnix-json v0.1.0 (/Users/aryehh/Development/rnix-json)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.73s
$ ifconfig lo0                                                      
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 16384
	options=1203<RXCSUM,TXCSUM,TXSTATUS,SW_TIMESTAMP>
	inet6 ::1 prefixlen 128 
	inet6 fe80::1%lo0 prefixlen 64 scopeid 0x1 
	nd6 options=201<PERFORMNUD,DAD>

abhillman avatar May 12 '24 23:05 abhillman

For good measure, here is the result I get with the parent commit:

$ cargo --version
cargo 1.80.0 (2f17770a1 2024-05-11)
$ cargo clippy --fix
error: failed to bind TCP listener to manage locking

Caused by:
  Can't assign requested address (os error 49)

abhillman avatar May 12 '24 23:05 abhillman

Looks great. I did also check it on linux, but I might recommend adding a regression test to help ensure that this works on other systems -- perhaps something like this? Fails without your patch: https://github.com/rust-lang/cargo/compare/master...abhillman:cargo:diag-server-regression-test. wdyt?

abhillman avatar May 13 '24 00:05 abhillman

Looks great. I did also check it on linux, but I might recommend adding a regression test to help ensure that this works on other systems -- perhaps something like? Fails without your patch: master...abhillman:cargo:diag-server-regression-test. wdyt?

Thanks, though I am not sure. What is the difference between this test and the entire end-to-end tests for cargo fix? The latter should always create a diagnostic server when exercising.

weihanglo avatar May 13 '24 00:05 weihanglo

Looks great. I did also check it on linux, but I might recommend adding a regression test to help ensure that this works on other systems -- perhaps something like? Fails without your patch: master...abhillman:cargo:diag-server-regression-test. wdyt?

Thanks, though I am not sure. What is the difference between this test and the entire end-to-end tests for cargo fix? The latter should always create a diagnostic server when exercising.

Ah! Thanks! I hadn't noticed these! Indeed, they fail without your patch and look more than sufficient 🚀

abhillman avatar May 13 '24 00:05 abhillman

Thanks! I have not tested this, but seems like it should work.

@bors r+

ehuss avatar May 20 '24 17:05 ehuss

:pushpin: Commit ced58bfd675264328040fc4f94cc2163c48358cf has been approved by ehuss

It is now in the queue for this repository.

bors avatar May 20 '24 17:05 bors

:hourglass: Testing commit ced58bfd675264328040fc4f94cc2163c48358cf with merge 3bbfe786a1f0efed0aa1653bc7015aaa27e79db6...

bors avatar May 20 '24 17:05 bors

:sunny: Test successful - checks-actions Approved by: ehuss Pushing 3bbfe786a1f0efed0aa1653bc7015aaa27e79db6 to master...

bors avatar May 20 '24 18:05 bors

:sunny: Test successful - checks-actions Approved by: ehuss Pushing 3bbfe786a1f0efed0aa1653bc7015aaa27e79db6 to master...

bors avatar May 20 '24 18:05 bors

:eyes: Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

bors avatar May 20 '24 18:05 bors