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

Torrust-Tracker won't compile on Windows #325

Open
Power2All opened this issue May 12, 2023 · 8 comments · May be fixed by #1098
Open

Torrust-Tracker won't compile on Windows #325

Power2All opened this issue May 12, 2023 · 8 comments · May be fixed by #1098
Assignees
Labels
- Contributor - Nice to support Torrust Build | Project System Compiling and Packaging Portability Distribution to More Places
Milestone

Comments

@Power2All
Copy link
Contributor

Just tried to compile this on Windows CLion with Rust.
These modules are not compatible with Windows:

  • ring
  • libz-sys
  • libsqlite3-sys
  • openssl-sys

You might want to look into replacing these, or looking to remove them.
Unless of course, you won't want to support multi-platforms.

@Power2All
Copy link
Contributor Author

Found the problem why it won't compile on Windows.
You use OpenSSL, which breaks.
Would advice to switch to Rustls, which does compile correctly on all platforms.

@josecelano
Copy link
Member

Found the problem why it won't compile on Windows. You use OpenSSL, which breaks. Would advice to switch to Rustls, which does compile correctly on all platforms.

Thank you @Power2All. I have to spend some time to understand the problem. Rigth now I only know:

I think we should:

  1. Decide which platforms we want to support.
  2. Make sure the app compiles for all of them.
  3. Add a workflow to check cross-compilation as I mentioned here.

I guess we want to support Windows unless there is a major performance issue or something like that. I think there is no active contributor using Windows for development right now.

cc @da2ce7 @WarmBeer

@Power2All
Copy link
Contributor Author

Power2All commented May 15, 2023

We changed #68 the openssl dependency to vendored.

I tried this too, but didn't work out as expected, as it's force to use MSVC, which you definitely don't want to use for compiling.

I've fixed all the issues on my end by skipping most of these -sys libraries it tries to use.
OpenSSL seems to be the only issue, it does this in combination with ring library, hence the issue.

I think there is no active contributor using Windows for development right now.

Before most of the changes, I was the one using Windows, with CLion as IDE.
Since I've been too busy with other projects, I wasn't paying attention on the changes on Torrust-Tracker (I've noticed RwLock is a performance hit, like a lot, compared when using Crossbeam Channel for mutex read and write actions, which I will be trying to work out anyway).

@da2ce7 da2ce7 moved this to Todo in Developer Experience Sep 21, 2023
@da2ce7 da2ce7 added this to the v3.0.0 milestone Sep 21, 2023
@da2ce7 da2ce7 added - Contributor - Nice to support Torrust Portability Distribution to More Places Build | Project System Compiling and Packaging labels Oct 10, 2023
@cgbosse cgbosse moved this to BUG & Security in Torrust Solution Jan 8, 2024
@cgbosse cgbosse moved this from BUG & Security to Feature & Enhancement request in Torrust Solution Jan 11, 2024
@cgbosse cgbosse moved this from Feature & Enhancement request to BUG & Security in Torrust Solution Jan 11, 2024
@cgbosse cgbosse modified the milestones: v3.0.0, v3.1.0 Jan 16, 2024
@GGLinnk
Copy link
Contributor

GGLinnk commented May 21, 2024

I investigated one of the 5 current test problems.

It seems that reqwest behaves differently under windows because of the way ports are managed.
But this only seems to have an impact on testing. The build actually runs without any problems (as far as I've tested).
Perhaps the ports are released somewhere in the tests, causing this problem.

Impacted test function: should_assign_to_the_peer_ip_the_remote_client_ip_instead_of_the_peer_address_in_the_request_param at

let status = client.announce(&announce_query).await.unwrap().status() ;

More information coming soon.

@GGLinnk
Copy link
Contributor

GGLinnk commented May 22, 2024

I've discovered that, for some unknown reason (yet), on Windows you can't use bind (it works using new).

Edit:
It seems that we can't use a LAN IP to the reqwest builder. I mean not the way it is currently made.
Maybe there is more things to do to get some kind of permission ?

@josecelano
Copy link
Member

I've discovered that, for some unknown reason (yet), on Windows you can't use bind, it works using new.

Edit: It seems that we can't use a LAN IP to the reqwest builder. I mean not the way it is currently made. Maybe there is more things to do to get some kind of permission ?

Hi @GGLinnk, can you paste the code that is not working? I don't know what "bind" or "new" methods you are referring to. Also, what LAN IP is not working? You can also add links to resources, related problems, or whatever you think might be helpful to help with this task.

@GGLinnk
Copy link
Contributor

GGLinnk commented May 23, 2024

Ok, for the context, I'm working on this part of the code :

#[tokio::test]
async fn should_assign_to_the_peer_ip_the_remote_client_ip_instead_of_the_peer_address_in_the_request_param() {
let env = Started::new(&configuration::ephemeral_mode_public().into()).await;
let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap();
let client_ip = local_ip().unwrap();
let announce_query = QueryBuilder::default()
.with_info_hash(&info_hash)
.with_peer_addr(&IpAddr::from_str("2.2.2.2").unwrap())
.query();
{
let client = Client::bind(*env.bind_address(), client_ip);
let status = client.announce(&announce_query).await.status();
assert_eq!(status, StatusCode::OK);
}
let peers = env.tracker.get_torrent_peers(&info_hash);
let peer_addr = peers[0].peer_addr;
assert_eq!(peer_addr.ip(), client_ip);
assert_ne!(peer_addr.ip(), IpAddr::from_str("2.2.2.2").unwrap());
env.stop().await;
}

This test creates a client using a bind method :

let client = Client::bind(*env.bind_address(), client_ip);

It uses two variables:

  • *env.bind_address(): Returns a locahost address (127.0.0.1) with a random port)
  • client_ip (local_ip().unwrap()): A local network address (mine was 192.168.1.28 for reference).

The bind methods creates a Client (Self) with the following reqwest builder :

reqwest: reqwest::Client::builder().local_address(local_address).build().unwrap(),

here local_address = client_ip

I've tried using the new method (and so, without the client_ip) and the test completed successfully.
For reference the new method is exactly the same just without the .local_address(local_address)

reqwest: reqwest::Client::builder().build().unwrap(),

I'm wondering what is the purpose to manually precise the local address here ? Is that required ?
Does the test passing is a good sign? Or does it requires reinforcement ?

@josecelano
Copy link
Member

josecelano commented May 24, 2024

Ok, for the context, I'm working on this part of the code :

#[tokio::test]
async fn should_assign_to_the_peer_ip_the_remote_client_ip_instead_of_the_peer_address_in_the_request_param() {
let env = Started::new(&configuration::ephemeral_mode_public().into()).await;
let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap();
let client_ip = local_ip().unwrap();
let announce_query = QueryBuilder::default()
.with_info_hash(&info_hash)
.with_peer_addr(&IpAddr::from_str("2.2.2.2").unwrap())
.query();
{
let client = Client::bind(*env.bind_address(), client_ip);
let status = client.announce(&announce_query).await.status();
assert_eq!(status, StatusCode::OK);
}
let peers = env.tracker.get_torrent_peers(&info_hash);
let peer_addr = peers[0].peer_addr;
assert_eq!(peer_addr.ip(), client_ip);
assert_ne!(peer_addr.ip(), IpAddr::from_str("2.2.2.2").unwrap());
env.stop().await;
}

This test creates a client using a bind method :

let client = Client::bind(*env.bind_address(), client_ip);

It uses two variables:

  • *env.bind_address(): Returns a locahost address (127.0.0.1) with a random port)
  • client_ip (local_ip().unwrap()): A local network address (mine was 192.168.1.28 for reference).

The bind methods creates a Client (Self) with the following reqwest builder :

reqwest: reqwest::Client::builder().local_address(local_address).build().unwrap(),

here local_address = client_ip

I've tried using the new method (and so, without the client_ip) and the test completed successfully. For reference the new method is exactly the same just without the .local_address(local_address)

reqwest: reqwest::Client::builder().build().unwrap(),

I'm wondering what is the purpose to manually precise the local address here ? Is that required ? Does the test passing is a good sign? Or does it requires reinforcement ?

Hi @GGLinnk I don't remember very well that part but I think I bound the local IP because I wanted to add this assert:

assert_eq!(peer_addr.ip(), client_ip);

The only way to know the client IP was to bind to a specific one. If I remember well, the client sometimes used different IPs. Maybe when you run it with docker (E2E tests), you get a different IP than the one you get running it directly.

It seems that in Windows, you cannot bind the client to the port returned using the local_ip function.

/// Retrieves the local IPv4 address of the machine in the local network from
/// the `AF_INET` family.
///
/// A different approach is taken based on the operative system.
///
/// For linux based systems the Netlink socket communication is used to
/// retrieve the local network interface.
///
/// For BSD-based systems the `getifaddrs` approach is taken using `libc`
///
/// For Windows systems Win32's IP Helper is used to gather the Local IP
/// address
pub fn local_ip() -> Result<IpAddr, Error> {

The test passes because the client's IP is the same as the one returned by local_ipbut that does not happen in some cases (probably the one I mentioned, docker).

You can try to run the E2E test in Windows to check if the tests also pass with new instead of bind:

cargo run --bin e2e_tests_runner ./share/default/config/tracker.e2e.container.sqlite3.toml

We could try to find a different way to know which IP the client is using instead of binding to a specific one. Maybe we can make another request before, and the server can return the client's IP, but I would not like to make this test depend on another feature.

@josecelano josecelano modified the milestones: v3.1.0, v3.0.0 Jun 12, 2024
@josecelano josecelano modified the milestones: v3.0.0, v3.1.0 Jul 10, 2024
@mario-nt mario-nt self-assigned this Nov 20, 2024
@josecelano josecelano linked a pull request Nov 22, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- Contributor - Nice to support Torrust Build | Project System Compiling and Packaging Portability Distribution to More Places
Projects
Status: BUG & Security
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants