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: support IPv6-only network for cargo fix #13907

Merged
merged 1 commit into from
May 20, 2024
Merged

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented May 12, 2024

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

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 IPv6 and IPv6
localhost addessses.
@rustbot
Copy link
Collaborator

rustbot commented May 12, 2024

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 rustbot added A-networking Area: networking issues, curl, etc. Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2024
@abhillman
Copy link

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
Copy link

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 pushed a commit to abhillman/cargo that referenced this pull request May 13, 2024
@abhillman
Copy link

abhillman commented May 13, 2024

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: master...abhillman:cargo:diag-server-regression-test. wdyt?

@weihanglo
Copy link
Member Author

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.

@abhillman
Copy link

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 🚀

@ehuss
Copy link
Contributor

ehuss commented May 20, 2024

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

@bors r+

@bors
Copy link
Collaborator

bors commented May 20, 2024

📌 Commit ced58bf has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2024
@bors
Copy link
Collaborator

bors commented May 20, 2024

⌛ Testing commit ced58bf with merge 3bbfe78...

@bors
Copy link
Collaborator

bors commented May 20, 2024

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 3bbfe78 to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented May 20, 2024

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 3bbfe78 to master...

@bors bors merged commit 3bbfe78 into rust-lang:master May 20, 2024
21 checks passed
@bors
Copy link
Collaborator

bors commented May 20, 2024

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

@weihanglo weihanglo deleted the ipv6 branch May 20, 2024 18:16
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Update cargo

9 commits in 0de7f2ec6c39d68022e6b97a39559d2f4dbf3930..84dc5dc11a9007a08f27170454da6097265e510e
2024-05-17 16:54:54 +0000 to 2024-05-20 18:57:08 +0000
- Fix warning about unused Permissions (rust-lang/cargo#13938)
- fix: support IPv6-only network for cargo fix (rust-lang/cargo#13907)
- Load `libsecret` by its `SONAME`, `libsecret-1.so.0` (rust-lang/cargo#13927)
- docs(ref): Simplify check-cfg build.rs docs (rust-lang/cargo#13937)
- Make `git::use_the_cli` test truly locale independent (rust-lang/cargo#13935)
- Silence warnings running embedded unittests. (rust-lang/cargo#13929)
- Fix warning output in build_with_symlink_to_path_dependency_with_build_script_in_git (rust-lang/cargo#13930)
- Fix:  Make path dependencies with the same name stays locked (rust-lang/cargo#13572)
- Temporarily fix standard_lib tests on linux. (rust-lang/cargo#13931)

r? ghost
@rustbot rustbot added this to the 1.80.0 milestone May 22, 2024
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 24, 2024
Update cargo

9 commits in 0de7f2ec6c39d68022e6b97a39559d2f4dbf3930..84dc5dc11a9007a08f27170454da6097265e510e
2024-05-17 16:54:54 +0000 to 2024-05-20 18:57:08 +0000
- Fix warning about unused Permissions (rust-lang/cargo#13938)
- fix: support IPv6-only network for cargo fix (rust-lang/cargo#13907)
- Load `libsecret` by its `SONAME`, `libsecret-1.so.0` (rust-lang/cargo#13927)
- docs(ref): Simplify check-cfg build.rs docs (rust-lang/cargo#13937)
- Make `git::use_the_cli` test truly locale independent (rust-lang/cargo#13935)
- Silence warnings running embedded unittests. (rust-lang/cargo#13929)
- Fix warning output in build_with_symlink_to_path_dependency_with_build_script_in_git (rust-lang/cargo#13930)
- Fix:  Make path dependencies with the same name stays locked (rust-lang/cargo#13572)
- Temporarily fix standard_lib tests on linux. (rust-lang/cargo#13931)

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Area: networking issues, curl, etc. Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diagnostic Server Fails on macOS 14.4
5 participants