Add support for compilation to windows #39

KolbyML · 2021-07-12T07:54:20Z

So currently two parts of the code only support Unix. Most notably is the interfaces library and currently, the std::net library doesn't support Unix domain socket server on windows which was added to windows in 2018. So I wrote windows equivalent functions using libraries that support windows. I also added a circleci build to test if Trin builds to Windows, I assume we could also add a cross-compile for MacOS in the future.

I think Trin being cross-platform would be important in the future if it ever hits production. Also I use Windows as my host platform so it would be nice if I could build trin natively.

Original post which was way too long: https://hastebin.com/opewikujef.sql

njgheorghita

Hey @KolbyML - nice to see you here! Stoked to get another contributor on the project. I'm also pretty new to Rust, so i'm still learning the best way to go about things. I left a couple of thoughts here on your code that should be worth a stab at refactoring.

Also, I have zero windows experience - so I'm afraid I can't help much in that area. I agree it'd be nice to have trin support multiple platforms. It might also be useful to look into some circleci mechanism / build tool that can make sure all merged code is cross-platform compatible, since I don't have an easy way to test the code I write. But, given my limited windows experience/knowledge, I'll leave it up to @carver to see what he thinks is the best way to go about cross-platform support.

src/jsonrpc.rs

njgheorghita · 2021-07-12T23:29:59Z

src/portalnet/socket.rs

+fn find_assigned_ip() -> Option<IpAddr> {
+    let adapters = ipconfig::get_adapters().unwrap();
+
+    for adapter in adapters.iter() {


For me, too many nested if statements can be a bit of a code smell. Especially in rust where they have quite a few handy mechanisms like .map() / .filter() / .is_empty() . I'd give this another go and see if you can trade out some of these nested ifs for something "flatter".

So I tried to work out a solution using a filter, but I just couldn't figure a good one out for this here is a picture of what I tried

You guys can problem come up with a better solution using a filter, but from what I could come up with using my limited knowledge of rust I think the first attempt I had at this problem was a lot cleaner, but maybe I just think that way cause I am just not as familiar with Rust.

Let me know what you think

I would of course remove the prints by the way just there cause I was testing

I'm also pretty new to rust, so it's tough for me to look at this and see a solution that's obviously better. Also, my compiler won't compile this code since I'm not on windows, which makes it harder since I usually like to tinker around with these things to see what does/doesn't work. This is the best I can do, but honestly, i'm not sure if it even works - but maybe it'll spark an idea on your end or maybe not.

ipconfig::get_adapters() .filter(|adapter| !adapter.gateways().is_empty()) .map(|adapter| adapter.ip_addresses()) .flatten() .find_map(|ip_addr| IpAddr::V4(_) = ip_addr)

Thanks I will take a look at it again tomorrow, from just looking at it, it seems like it will work. I haven't seen flatten() before so I will read up on it

Hopefully it works haha, yeah I hadn't seen it before either - but just came across it while browsing https://doc.rust-lang.org/std/iter/trait.Iterator.html#provided-methods hoping to stumble across something useful

So I ended up trying to take another look at it and this is the best I could come up with using filter and such. Let me know if you want me to commit this or keep the first thing I came up with

I guess I will start looking at cl now

njgheorghita

Cool! Maybe give the flat approach one more try - but if nothing works, I think the nested approach should be ok in this scenario.

Though, I'm a little nervous about the added complexity cross-platform support introduces to our code. If we go ahead, I don't have a way to test that the code I write is cross-compatible. But, maybe there's a CI solution that'll help?

OK - just found this - https://github.com/rust-embedded/cross - seems like it'll solve my concerns and is worth integrating into our workflow / ci if we go ahead with cross-platform support. @carver any thoughts?

njgheorghita · 2021-07-14T02:05:07Z

src/portalnet/socket.rs

+fn find_assigned_ip() -> Option<IpAddr> {
+    let adapters = ipconfig::get_adapters().unwrap();
+
+    for adapter in adapters.iter() {


I'm also pretty new to rust, so it's tough for me to look at this and see a solution that's obviously better. Also, my compiler won't compile this code since I'm not on windows, which makes it harder since I usually like to tinker around with these things to see what does/doesn't work. This is the best I can do, but honestly, i'm not sure if it even works - but maybe it'll spark an idea on your end or maybe not.

ipconfig::get_adapters() .filter(|adapter| !adapter.gateways().is_empty()) .map(|adapter| adapter.ip_addresses()) .flatten() .find_map(|ip_addr| IpAddr::V4(_) = ip_addr)

njgheorghita · 2021-07-14T02:05:37Z

src/jsonrpc.rs

+#[cfg(windows)]
+fn get_listener_result(ipc_path: &str) -> tokio::io::Result<uds_windows::UnixListener> {
+    uds_windows::UnixListener::bind(ipc_path)
+}


Nice, I think this is definitely an improvement!

KolbyML · 2021-07-14T06:11:10Z

Cool! Maybe give the flat approach one more try - but if nothing works, I think the nested approach should be ok in this scenario.

Though, I'm a little nervous about the added complexity cross-platform support introduces to our code. If we go ahead, I don't have a way to test that the code I write is cross-compatible. But, maybe there's a CI solution that'll help?

I will look into the first thing and also I will look into Cl. I know Bitcoins travis builds compile for all platforms, so we can probably do something like that with Cl

KolbyML · 2021-07-14T23:41:17Z

Ok I just added a ci job to cross-compile the windows build

njgheorghita · 2021-07-15T22:59:48Z

Let me know if you want me to commit this or keep the first thing I came up with

In this case, I'd agree that the nested approach seems cleaner.

CI's also looking good. But it seems now that you'll need to rebase this branch against master to get rid of the merge conflicts. Since, I'm fairly unfamiliar with windows and new to rust, I'd still like to get another pair of 👀 on this before merging if @carver or @SamWilsn get the chance

KolbyML · 2021-07-16T17:58:33Z

CI's also looking good. But it seems now that you'll need to rebase this branch against master to get rid of the merge conflicts. Since, I'm fairly unfamiliar with windows and new to rust, I'd still like to get another pair of 👀 on this before merging if @carver or @SamWilsn get the chance

Ok so I rewrote the CI to compile natively on Windows Server for the build because cross-compilation is no longer supported to windows because of the addition of RocksDB as it doesn't support the gnu toolchain for windows.

Anyways compiling it natively is ideal for testing the windows build because then you can run cargo test on it, I was planning to do it later anyway.

carver

Sweet, it's great to have a windows build! Sorry it took so long to get my eyes on it. :/

Note that we often give a sort of "best-effort" approval on the team, which means "After you make the mentioned changes, using your best judgement, go ahead and merge. I will only review again if you ask me to." Request Changes is only used if I specifically want to comment on your new implementation before merging.

carver · 2021-07-21T19:08:15Z

.circleci/config.yml

-                # Force a failure on clippy warnings
-                flags: "-- --deny warnings"
-                with_cache: false
+            # Run the commands without using the orbs since Clippy needs a little longer then 10 minutes


This note shocked me (that clippy needs so much time). After digging in, it looks like it's really the build time that is taking so long. The note would have been clearer to me if it said something like:

Suggested change

# Run the commands without using the orbs since Clippy needs a little longer then 10 minutes

# Run the sub-commands of the orb jobs separately, since the combined steps for Clippy can be long enough to trigger a timeout (>10min). Building the source is the main contributor.

carver · 2021-07-21T19:09:52Z

.circleci/config.yml

+      - run:
+          name: Install target toolchain
+          command: rustup toolchain install stable-x86_64-pc-windows-msvc
+      # Run the commands without using the orbs since Clippy needs a little longer then 10 minutes


Same thing here, naturally.

carver · 2021-07-21T19:11:23Z

.circleci/config.yml

+          command: rustup component add clippy
+      - run:
+          name: Run Clippy
+          # Remove the first two lines of gitconfig as work around


As a workaround for what?

rust-lang/cargo#2078
rust-lang/cargo#2422
https://app.circleci.com/pipelines/github/carver/trin/147/workflows/d059c7c6-d759-4f79-a96c-99c16ba078ab/jobs/177

Here is some context on it.

Updating crates.io index error: failed to get `base64` as a dependency of package `trin v0.1.0 (C:\Users\circleci\project)` Caused by: failed to load source for dependency `base64` Caused by: Unable to update registry `https://github.com/rust-lang/crates.io-index` Caused by: failed to fetch `https://github.com/rust-lang/crates.io-index` Caused by: failed to authenticate when downloading repository: ssh://git@github.com/rust-lang/crates.io-index * attempted ssh-agent authentication, but no usernames succeeded: `git` if the git CLI succeeds then `net.git-fetch-with-cli` may help here https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli Caused by: error authenticating: failed connecting agent; class=Ssh (23)

So that was the "fix" I could find from googling it.

carver · 2021-07-21T19:13:36Z

src/portalnet/socket.rs

+
+#[cfg(windows)]
+fn find_assigned_ip() -> Option<IpAddr> {
+    let adapters = ipconfig::get_adapters().unwrap();


Presumably unwrap is panicking here on a failure. I think we want to return None instead.

KolbyML closed this Jul 12, 2021

KolbyML reopened this Jul 12, 2021

njgheorghita requested changes Jul 12, 2021

View reviewed changes

KolbyML requested a review from njgheorghita July 13, 2021 04:49

njgheorghita approved these changes Jul 14, 2021

View reviewed changes

njgheorghita requested a review from carver July 15, 2021 23:00

KolbyML closed this Jul 16, 2021

KolbyML reopened this Jul 16, 2021

carver approved these changes Jul 21, 2021

View reviewed changes

Add support for compilation to windows

21052ac

KolbyML merged commit bb8a935 into ethereum:master Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for compilation to windows #39

Add support for compilation to windows #39

KolbyML commented Jul 12, 2021 •

edited

Loading

njgheorghita left a comment

njgheorghita Jul 12, 2021

KolbyML Jul 13, 2021

KolbyML Jul 13, 2021

njgheorghita Jul 14, 2021

KolbyML Jul 14, 2021

njgheorghita Jul 14, 2021

KolbyML Jul 14, 2021 •

edited

Loading

njgheorghita left a comment

njgheorghita Jul 14, 2021

njgheorghita Jul 14, 2021

KolbyML commented Jul 14, 2021

KolbyML commented Jul 14, 2021

njgheorghita commented Jul 15, 2021

KolbyML commented Jul 16, 2021 •

edited

Loading

carver left a comment

carver Jul 21, 2021

carver Jul 21, 2021

carver Jul 21, 2021

KolbyML Jul 21, 2021

carver Jul 21, 2021

	# Run the commands without using the orbs since Clippy needs a little longer then 10 minutes
	# Run the sub-commands of the orb jobs separately, since the combined steps for Clippy can be long enough to trigger a timeout (>10min). Building the source is the main contributor.

Add support for compilation to windows #39

Add support for compilation to windows #39

Conversation

KolbyML commented Jul 12, 2021 • edited Loading

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Jul 14, 2021

KolbyML commented Jul 14, 2021

njgheorghita commented Jul 15, 2021

KolbyML commented Jul 16, 2021 • edited Loading

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Jul 12, 2021 •

edited

Loading

KolbyML Jul 14, 2021 •

edited

Loading

KolbyML commented Jul 16, 2021 •

edited

Loading